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

Chore: Improvement of tests script to allow param passing (Issue/3472) #3478

Merged
merged 16 commits into from
Dec 20, 2023

Conversation

cahirodoherty-learningpool
Copy link
Contributor

Addresses part 1 and part 2 of #3472

  • Split up the test prepare command to allow only install/only grunt diff based on input and/or state
  • Allowed for passing of a limited set of arguments to the test command

@cahirodoherty-learningpool
Copy link
Contributor Author

Note on this - I had considered creating a 'utils' folder and splitting out some functions to a testHelpers file but felt it was potentially a step too far for this PR and also something worth getting peoples thoughts on. I'll happily add this or extend from this branch and do that if it is desired. @oliverfoster Any thoughts there?

@oliverfoster
Copy link
Member

Note on this - I had considered creating a 'utils' folder and splitting out some functions to a testHelpers file but felt it was potentially a step too far for this PR and also something worth getting peoples thoughts on. I'll happily add this or extend from this branch and do that if it is desired. @oliverfoster Any thoughts there?

I think it's a waste of time and space unless the functions are going to be used elsewhere.

test.js Show resolved Hide resolved
test.js Outdated Show resolved Hide resolved
test.js Outdated Show resolved Hide resolved
test.js Show resolved Hide resolved
@cahirodoherty-learningpool
Copy link
Contributor Author

Hi @oliverfoster thanks for your feedback on this. Do you feel the null default is suitable for the testfiles argument and trust in any user looking to specify a subset that they will provide the correct string(s)?
Or do you think we need to split the arguments into two; one for unittestfiles and one for e2etestfiles?

@oliverfoster
Copy link
Member

Hi @oliverfoster thanks for your feedback on this. Do you feel the null default is suitable for the testfiles argument and trust in any user looking to specify a subset that they will provide the correct string(s)? Or do you think we need to split the arguments into two; one for unittestfiles and one for e2etestfiles?

I imagine it's only going to be used when writing tests, to specify which tests to run. Is there any other use-case so far?

@cahirodoherty-learningpool
Copy link
Contributor Author

I imagine it's only going to be used when writing tests, to specify which tests to run. Is there any other use-case so far?

No I think thats all. I feel if we meet any use cases in the future we can amend to meet 🤷‍♂️

Thanks for the approval 👍

@oliverfoster oliverfoster merged commit 04975de into master Dec 20, 2023
2 checks passed
@oliverfoster oliverfoster deleted the issue/3472 branch December 20, 2023 15:42
github-actions bot pushed a commit that referenced this pull request Dec 20, 2023
## [5.33.12](v5.33.11...v5.33.12) (2023-12-20)

### Fix

* Improvement of tests script to allow param passing (Issue/3472) (#3478) ([04975de](04975de)), closes [#3478](#3478)
Copy link

🎉 This PR is included in version 5.33.12 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants