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

Run Teardown after each test #1818

Merged
merged 6 commits into from
Feb 26, 2021

Conversation

jvanbaarsen
Copy link
Contributor

We currently run Teardown at the end of a scenario, but whenever we fail
to load in a scenario the tests are erroring and we don't perform the
cleanup

Fixes: #1815

We currently run Teardown at the end of a scenario, but whenever we fail
to load in a scenario the tests are erroring and we don't perform the
cleanup

Fixes: redwoodjs#1815
@jvanbaarsen
Copy link
Contributor Author

I need to test this against a PR package since the local RWT flow is broken at the moment.

@github-actions
Copy link

github-actions bot commented Feb 19, 2021

📦 PR Packages

Click to Show Package Download Links

https://rw-pr-redwoodjs-com.s3.amazonaws.com/1818/create-redwood-app-0.26.0-cd119e1.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1818/redwoodjs-api-0.26.0-cd119e1.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1818/redwoodjs-api-server-0.26.0-cd119e1.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1818/redwoodjs-auth-0.26.0-cd119e1.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1818/redwoodjs-cli-0.26.0-cd119e1.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1818/redwoodjs-core-0.26.0-cd119e1.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1818/redwoodjs-dev-server-0.26.0-cd119e1.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1818/redwoodjs-eslint-config-0.26.0-cd119e1.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1818/redwoodjs-eslint-plugin-redwood-0.26.0-cd119e1.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1818/redwoodjs-forms-0.26.0-cd119e1.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1818/redwoodjs-internal-0.26.0-cd119e1.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1818/redwoodjs-prerender-0.26.0-cd119e1.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1818/redwoodjs-router-0.26.0-cd119e1.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1818/redwoodjs-structure-0.26.0-cd119e1.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1818/redwoodjs-testing-0.26.0-cd119e1.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1818/redwoodjs-web-0.26.0-cd119e1.tgz

Install this PR by running yarn rw upgrade --pr 1818:0.26.0-cd119e1

@dac09
Copy link
Collaborator

dac09 commented Feb 19, 2021

I don't have full context of this, but wouldn't it make more sense to use afterAll ? I imagine that you would setup a fixture for each describe block, rather than each it block

@jvanbaarsen
Copy link
Contributor Author

@dac09 You want to reload the scenario's on a per test basis. Otherwise they start to interfere with each other.

@thedavidprice
Copy link
Contributor

Thanks for this @jvanbaarsen This looks good to me and I'd like to get it into the v0.26.0 release. Will loop in @cannikin for the final ok.

but whenever we fail
to load in a scenario the tests are erroring and we don't perform the cleanup

^^ Do either of you have any suggestions about how to test for this. And/or could you confirm you've tested this locally and it's working as intended?

@cannikin
Copy link
Member

Yeah if you can confirm this is working as planned it looks good to me @jvanbaarsen Were you ever able to test it against a PR release?

Does this take care of the case where a scenario seed fails? We've got an open issue #1804 to address that, but this change might fix it. One way to test is to add another model to the scenario that doesn't exist:

const standard = defineScenario({
  post: {
    one: {
      title: 'First post'
    }
  },
  foo: 'bar'
})

That should throw an error like can't call method create on undefined or something similar. But afterEach() should be anticipating errors and still executing, I hope?

/cc @agiannelli this PR might have coincidentally fixed your issue as well!

@jvanbaarsen
Copy link
Contributor Author

jvanbaarsen commented Feb 24, 2021

@thedavidprice / @cannikin Please hold back a bit with merging this. I haven't been able to properly test this just yet (Should have put this back in Draft -- edit: Turns out I'm not able to return this as a draft 🙈 )

@thedavidprice thedavidprice marked this pull request as draft February 24, 2021 17:38
@jvanbaarsen jvanbaarsen marked this pull request as ready for review February 25, 2021 20:26
@jvanbaarsen
Copy link
Contributor Author

jvanbaarsen commented Feb 25, 2021

Ready for review!

You can test this by running the following steps:

  • Create a new project, and create a service using rw g service UserExample (We are using the default table in schema.prisma for this)

  • Run rw test api, all should be green

  • Update the UserExample scenario to this:

    export const standard = defineScenario({
      userExample: {
        one: { email: 'String7883988' },
        two: { email: 'String1051631' },
      },
      foo: 'bar',
    })
    
  • Run tests again, you should get an error

  • Run it again and you should still see the same error (If you don't get a Unique Constraint error this fix works)

@cannikin
Copy link
Member

Sweet! So this should close both #1815 AND #1805, correct? Whether your own code fails, or the seeding of the scenario fails, the database is cleared out looks like.

@jvanbaarsen
Copy link
Contributor Author

Yep! Sorry @agiannelli I wasn’t aware that these issues were overlapping :(

@agiannelli
Copy link
Contributor

@jvanbaarsen no need to apologize! I'm still working on getting my local up and running anyway 🙈

@jvanbaarsen
Copy link
Contributor Author

@agiannelli Let me know if you need any help with that 👍

@thedavidprice
Copy link
Contributor

Hi All! I'm looking into this now as a potential patch release. brb!!

@cannikin
Copy link
Member

patch it!

@cannikin
Copy link
Member

In this GIF metaphor the water is the extra records left around in the database.

@cannikin
Copy link
Member

This closes #1804 not #1805!

@thedavidprice thedavidprice added this to the v0.26.1 milestone Feb 26, 2021
@jvanbaarsen jvanbaarsen deleted the 1815-afterEachTeardown branch March 1, 2021 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Services tests run outside of our scenario() function will not have their database records rolled back
5 participants