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

Fix warnings in CLI destroy tests #6455

Merged
merged 5 commits into from
Sep 27, 2022

Conversation

Josh-Walker-GM
Copy link
Collaborator

@Josh-Walker-GM Josh-Walker-GM commented Sep 26, 2022

Problem
See #6450 which discusses a warning for the destroy tests related to cells.

I was able to replicate the issue locally when restricting the number of threads used within the test:

yarn jest src/commands/destroy/cell/__tests__/cell.test.js --maxWorkers=1

Solution
Ensure async functions are awaited.

Changes

  1. Removed file mocking within beforeEach as it's not needed before each test given the second test needs different files mock.
  2. await all files(...) calls
  3. await listr task run before expecting tests

Questions

  1. Other tests e.g. page seem to also not await files - this is also an issue?
  2. Happy with the async/await approach over then's?

@dac09 dac09 added the release:chore This PR is a chore (means nothing for users) label Sep 26, 2022
@dac09
Copy link
Collaborator

dac09 commented Sep 26, 2022

Thanks @Josh-Walker-GM - found this while reviewing your PR - much appreciated!

Co-authored-by: Daniel Choudhury <dannychoudhury@gmail.com>
@dac09
Copy link
Collaborator

dac09 commented Sep 26, 2022

Other tests e.g. page seem to also no await files - this is also an issue?

Yes ideally we can move this over to the syntax you're using! I tried this earlier today but didn't have enough time to debug why my changes made the tests fail!

Happy with the async/await approach over then's?

Definitely 💫

@Josh-Walker-GM
Copy link
Collaborator Author

Thanks @Josh-Walker-GM - found this while reviewing your PR - much appreciated!

Thought I should have a go fixing the issue given my changes highlighted/caused it 😆

@Josh-Walker-GM
Copy link
Collaborator Author

If you wish I can take a look at updating the remaining tests so they await the files and other async calls? This is best in another PR but is it okay if I update all the remaining tests within that one PR?

@Josh-Walker-GM
Copy link
Collaborator Author

Josh-Walker-GM commented Sep 26, 2022

Other tests e.g. page seem to also no await files - this is also an issue?

Yes ideally we can move this over to the syntax you're using! I tried this earlier today but didn't have enough time to debug why my changes made the tests fail!

In theory this could lead to tests passing without the correct behaviour, no?

Copy link
Collaborator

dac09 commented Sep 26, 2022

Yes - that’s right 🙂. Are you happy to do the changes in this PR? Then I’ll wait to merge

@Josh-Walker-GM
Copy link
Collaborator Author

Very happy to update the rest within this PR.

I hopefully won't run into any tests which currently pass due to the async but in theory shouldn't when sync'd. If I run into any difficulties that I can't solve I'll update here too if that's fine?

Copy link
Collaborator

dac09 commented Sep 26, 2022

Yes ofcourse, that’s great. I think the other destroy commands don’t have the async problem anyway, so should be straightforward!

@Josh-Walker-GM
Copy link
Collaborator Author

I did a pass through the tests and a few local runs and didn't notice any other issues with the tests. I removed an unneeded async but that's it.

Copy link
Collaborator

@dac09 dac09 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you again Josh, on 🔥

@dac09 dac09 enabled auto-merge (squash) September 27, 2022 10:45
@dac09 dac09 merged commit fbfd467 into redwoodjs:main Sep 27, 2022
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Sep 27, 2022
@jtoar jtoar modified the milestones: next-release, v3.1.0 Sep 28, 2022
@Josh-Walker-GM Josh-Walker-GM deleted the cli-tests-cell-destroy branch October 2, 2022 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:chore This PR is a chore (means nothing for users)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants