Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Best way to tear down Step classes and their dependencies? #993

Closed
RichardBradley opened this issue Apr 22, 2016 · 9 comments
Closed

Best way to tear down Step classes and their dependencies? #993

RichardBradley opened this issue Apr 22, 2016 · 9 comments

Comments

@RichardBradley
Copy link
Contributor

RichardBradley commented Apr 22, 2016

Hi,

I have a medium / large webapp using cucumber-jvm for integration testing.

We have 38 Step definition classes (one for each feature group, and a few common step groups), which use 23 "connector" type dependency classes. The connectors do things like connecting directly to the database of the app under test for steps like "given I have a widget of type X".

The step code looks a bit like:

public class WidgetSteps implements En {
   private final DatabaseTestConnector database;
   private final List<Widget> widgetsToCleanup = new ArrayList<>();

   public WidgetSteps(DatabaseTestConnector database) {
      this.database = database;
      defineSteps(); // (separate method to avoid #916)
   }

   private void defineSteps() {
      Given("^there is a widget of type X$", () -> {
         widgetsToCleanup.add(database.createWidget("x"));
      }
   }

   @After
   private void cleanup() {
      widgetsToCleanup.foreach(database::deleteWidget);
   }
}

public class DatabaseTestConnector {
  private final Connection connection;

  public DatabaseTestConnector() {
    connection = connectToDatabase(); // creates a TCP connection which needs closing!
  }

  public void deleteWidget(Widget w) { .. }

  public Widget createWidget(...) { ... }

  @After // (often runs in the wrong order!)
  public void cleanUp() {
    connection.close();
  }
}

We are using cucumber-picocontainer as recommended at https://cucumber.io/docs/reference/java-di

I have recently noticed that cucumber-jvm creates a new set of Step classes for every single Scenario, as discussed at http://stackoverflow.com/a/33063300/8261 . It also creates all Step classes (and their dependencies) for all scenarios, even if they are not needed.

This is fairly inefficient for us, as we are forever creating and tearing down database connections even for Scenarios that don't use the database. I have refactored our connectors to create connections lazily to help with this.

At the same time, I noticed that we were leaking loads of database connections because of there being one instance of DatabaseTestConnector for each Scenario. As noted at #992, the Picocontainer used by cucumber-jvm is configured to not use its Lifecycle annotations. I tried to fix the leak by adding the "@after" annotations to DatabaseTestConnector as shown above, but it seems that the order of @after annotations is not guaranteed or stable, so my DB connection is now sometimes closed before the Step tries to do its cleanup.

So:

  1. Am I doing anything obviously wrong here? Is there a better way to set all this up?
  2. Can I do anything to enforce that the @after on DatabaseTestConnector runs after the @after on WidgetSteps?
  3. Is it a bug that the cucumber-jvm picocontainer is configured with a NullLifecycleStrategy? I think the easiest fix here would be to change that and to use Disposeable on DatabaseTestConnector instead of @after.
  4. Can I specify that these two support classes are safe to reuse for multiple Scenarios and save the cost of recreating them each time?

Thanks

@RichardBradley
Copy link
Contributor Author

I have created a pull request to address item 3 above: #994

@jwgmeligmeyling
Copy link

jwgmeligmeyling commented May 15, 2016

Hi @RichardBradley ,

We've been using cucumber-jvm for a while now and I've been really struck by the design issues related to the @Before and @After annotations. They really should only be invoked if you use a step definition from that step definition class file. Is there an (separate) open issue for this already? Or is this part of this issue?

See also the discussion we've had at our software testing course on Cucumber usage of @After and @Before's : SERG-Delft/jpacman-framework#92

@RichardBradley
Copy link
Contributor Author

@Before and @After annotations .. really should only be invoked if you use a step definition from that step definition class file. ... Is there an (separate) open issue for this already?

That's a separate issue to the one I'm discussing here, which is about how to ensure that @After steps are run in a consistent dependency-order.

To avoid derailing this issue, please could you raise a new issue about that? If you CC me on there, I'll add my thoughts about it.

@dkowis
Copy link
Member

dkowis commented May 22, 2016

merged as of c0ffacd

@dkowis dkowis closed this as completed May 22, 2016
@RichardBradley
Copy link
Contributor Author

I had 4 questions in my comment above.

2 & 3 have been fixed by the commits you merged (or have nearly merged, see my comments on #994).

@dkowis , do you have any thoughts on 1 & 4 above?

@brasmusson
Copy link
Contributor

Can I do anything to enforce that the @After on DatabaseTestConnector runs after the @After on WidgetSteps?

The @Before and @After annotations can take an order attribute which is used to control the order of execution (example the Spring transaction hooks has the order 100).

The @Before hooks are executed in ascending order, and the @After hooks are executed in descending order as defined by the order attribute, see this test

Can I specify that these two support classes are safe to reuse for multiple Scenarios and save the cost of recreating them each time?

No, not as far as I know.

@dkowis
Copy link
Member

dkowis commented May 25, 2016

I'm afraid I don't have any advice here, I generally have avoided using injection systems in my cucumber step definitions :) Sorry.

@RichardBradley
Copy link
Contributor Author

OK, thanks. The changes in my PR have fixed this enough for me, and hopefully the extra docs will suffice for anyone with similar issues in future.

@lock
Copy link

lock bot commented Oct 25, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants