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

move test reinit to mocha's after hooks #557

Closed

Conversation

alxndrsn
Copy link
Contributor

@alxndrsn alxndrsn commented Jul 30, 2022

Closes #499

What has been done to verify that this works as intended? CI

Why is this the best possible solution? Were any other approaches considered? #552

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks? Does not affect users.

Does this change require updates to the API documentation? If so, please update docs/api.md as part of this PR.

Before submitting this PR, please make sure you have:

  • run make test-full and confirmed all checks still pass OR confirm CircleCI build passes
  • verified that any code from external sources are properly credited in comments

@matthew-white
Copy link
Member

This seems like a nice approach in many ways! The main downside I see is that the output will be more nested and a little more verbose, since withinFullTrx() will put each test within its own describe() call.

It'd be awesome if Mocha let us somehow add an afterEach hook for the one test without putting the whole thing in a describe() call. However, from a brief look, it doesn't seem like Mocha supports that.

I'm curious what you think about my approach at #552 (comment). It defines a flag via let, but that's isolated within test/integration/setup.js. I think that approach would allow existing tests to stay the same: only test/integration/setup.js would have to change.

Whatever the ultimate approach is, a change in this area seems very useful. But I'm realizing that I don't quite understand how it closes #499. Making this change will make it less likely for a test timeout to cause the next test to fail or timeout. But the situation in #499 is different from that, because there there's only one test that fails.

I've put these questions on the agenda for the meeting tomorrow in case it's helpful to discuss further then!

@matthew-white
Copy link
Member

I think this can be closed now that #558 is merged.

@alxndrsn alxndrsn deleted the test-reinit-describe-wrapper branch August 4, 2022 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants