-
Notifications
You must be signed in to change notification settings - Fork 15
feat(cli): add --persist.filename
cli option
#187
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but this PR is doing way too much. Please reduce the scope. I don't want to merge all these changes.
- Sure, add
--persist.filename
as a CLI args also. - Yes, use the timestamp to avoid multiple tests interacting with the same file.
- Everything else should be refined first.
Co-authored-by: Matěj Chalk <[email protected]>
Co-authored-by: Matěj Chalk <[email protected]>
@matejchalk I suggest we switch in the future to timestamp or hash based filenames. If you agree I would keep the logic for filename generation in tests and the logic. WDYT? |
I think that Timestamp would be more useful than hash, I think, but I'm not sure it's a good idea at this point. And the idea with allowing placeholder syntax in the
Also, I think anything other than changing adding |
So lets move the comments into a new issue to discuss and I remove the testing logic. |
Co-authored-by: Matěj Chalk <[email protected]>
Did my best to reduce scope from the refactoring. Next PR is on testing lib so this will get better. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did my best to reduce scope from the refactoring. Next PR is on testing lib so this will get better.
Thanks, it's a lot better now. Added some small code style comments (we have lint rules for the unnecessary async
s and missing await
s, BTW, just wasn't able to turn it on yet). But much happier with the scope now 🙂
Co-authored-by: Matěj Chalk <[email protected]>
Co-authored-by: Matěj Chalk <[email protected]>
Co-authored-by: Matěj Chalk <[email protected]>
Co-authored-by: Matěj Chalk <[email protected]>
Co-authored-by: Matěj Chalk <[email protected]>
Co-authored-by: Matěj Chalk <[email protected]>
Co-authored-by: Matěj Chalk <[email protected]>
This PR is a followup on #174 and aims to fix the flaky tests related to same report file names.
report.json
as is can cause overwrites when running multiple times in a row (e.g. runtime assertions).It includes:
persist.format
CLI optionsPotential enhancement for another consideration when we more on that:
From LHCI:
closes #161