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

feat(cli): add --persist.filename cli option #187

Merged
merged 35 commits into from
Nov 10, 2023
Merged

Conversation

BioPhoton
Copy link
Collaborator

@BioPhoton BioPhoton commented Oct 30, 2023

This PR is a followup on #174 and aims to fix the flaky tests related to same report file names.

  • The default name of a report can't be report.json as is can cause overwrites when running multiple times in a row (e.g. runtime assertions).
  • for multiple runs we should group the runs somehow
  • the report name can be custom
  • the upload command could upload multiple reports

It includes:

  • persist.format CLI options
  • loadReports helper
  • test refactoring
  • makes the test setup pretty stable (one thing that is missing is a refactor of the static config files to have unique file names as the rest)

Potential enhancement for another consideration when we more on that:

From LHCI:

{
    reportFilenamePattern: {
      type: 'string',
      description: '[filesystem only] The pattern to use for naming Lighthouse reports.',
      default: '%%HOSTNAME%%-%%PATHNAME%%-%%DATETIME%%.report.%%EXTENSION%%',
    }
}

closes #161

@BioPhoton BioPhoton self-assigned this Oct 31, 2023
@BioPhoton BioPhoton added 🤓 UX UX improvement for CLI users 🔬 testing writing tests 🧩 cli labels Oct 31, 2023
@BioPhoton BioPhoton added this to the 1. Internal MVP milestone Oct 31, 2023
Copy link
Collaborator

@matejchalk matejchalk left a 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.

e2e/cli-e2e/tests/collect.spec.ts Outdated Show resolved Hide resolved
packages/core/src/lib/implementation/persist.spec.ts Outdated Show resolved Hide resolved
packages/utils/src/lib/utils.ts Outdated Show resolved Hide resolved
packages/utils/src/lib/report.ts Outdated Show resolved Hide resolved
packages/models/src/lib/implementation/utils.ts Outdated Show resolved Hide resolved
packages/core/src/lib/upload.ts Outdated Show resolved Hide resolved
packages/models/src/lib/persist-config.ts Outdated Show resolved Hide resolved
e2e/cli-e2e/mocks/fs.mock.ts Outdated Show resolved Hide resolved
BioPhoton and others added 4 commits November 4, 2023 22:18
Co-authored-by: Matěj Chalk <34691111+matejchalk@users.noreply.github.com>
Co-authored-by: Matěj Chalk <34691111+matejchalk@users.noreply.github.com>
e2e/cli-e2e/tests/print-config.spec.ts Outdated Show resolved Hide resolved
packages/core/src/lib/implementation/persist.spec.ts Outdated Show resolved Hide resolved
packages/core/src/lib/upload.spec.ts Outdated Show resolved Hide resolved
packages/models/src/lib/implementation/utils.spec.ts Outdated Show resolved Hide resolved
packages/models/src/lib/persist-config.ts Outdated Show resolved Hide resolved
packages/utils/src/lib/report.ts Outdated Show resolved Hide resolved
packages/core/src/lib/upload.ts Outdated Show resolved Hide resolved
packages/utils/src/lib/report.ts Outdated Show resolved Hide resolved
packages/utils/src/lib/utils.ts Outdated Show resolved Hide resolved
@BioPhoton
Copy link
Collaborator Author

@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?

@matejchalk
Copy link
Collaborator

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 report.json is a good and simple default for our end users. Being able to change the filename is something that's useful mainly for our tests - but only those that interact with the real file-system, tests using memfs can stick to the default, I believe.

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 persist.filename option would then make sense, but there are unintended consequences I would prefer to avoid. E.g. the upload command would then have to resolve the filename dynamically and can have more than one match. But uploading multiple reports makes no sense at present and introduces unexpected behavior:

  • the upload command takes the current commit as the ID it send to the portal, so multiple reports would get matched to same commit ID and thus the "multi-upload" will result in only one report (the last one if uploaded in sequence, who knows which one if uploaded in parallel 😬)
  • we don't prompt the user to clean their persist.outputDir, which could have intended consequences of re-uploaded the same report when running collect and upload multiple times

Also, I think anything other than changing adding --persist.filename and using it to fix test flakiness should be out of scope of this PR. Changing the default and changing the single upload to a multi-upload ... it changes way too much and I'm not sure it's for the better.

@BioPhoton
Copy link
Collaborator Author

So lets move the comments into a new issue to discuss and I remove the testing logic.

@BioPhoton
Copy link
Collaborator Author

Did my best to reduce scope from the refactoring. Next PR is on testing lib so this will get better.

Copy link
Collaborator

@matejchalk matejchalk left a 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 asyncs and missing awaits, BTW, just wasn't able to turn it on yet). But much happier with the scope now 🙂

e2e/cli-e2e/tests/print-config.spec.ts Outdated Show resolved Hide resolved
packages/models/src/index.ts Outdated Show resolved Hide resolved
packages/utils/src/lib/report.spec.ts Outdated Show resolved Hide resolved
packages/utils/src/lib/report.ts Outdated Show resolved Hide resolved
packages/utils/src/lib/utils.spec.ts Outdated Show resolved Hide resolved
packages/utils/src/lib/utils.spec.ts Outdated Show resolved Hide resolved
packages/utils/src/lib/utils.ts Outdated Show resolved Hide resolved
packages/core/src/lib/implementation/persist.ts Outdated Show resolved Hide resolved
BioPhoton and others added 9 commits November 10, 2023 19:06
Co-authored-by: Matěj Chalk <34691111+matejchalk@users.noreply.github.com>
Co-authored-by: Matěj Chalk <34691111+matejchalk@users.noreply.github.com>
Co-authored-by: Matěj Chalk <34691111+matejchalk@users.noreply.github.com>
Co-authored-by: Matěj Chalk <34691111+matejchalk@users.noreply.github.com>
Co-authored-by: Matěj Chalk <34691111+matejchalk@users.noreply.github.com>
Co-authored-by: Matěj Chalk <34691111+matejchalk@users.noreply.github.com>
Co-authored-by: Matěj Chalk <34691111+matejchalk@users.noreply.github.com>
@BioPhoton BioPhoton merged commit 296df7d into main Nov 10, 2023
9 checks passed
@BioPhoton BioPhoton deleted the add-filename-cli-arg branch November 10, 2023 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧩 cli 🔬 testing writing tests 🤓 UX UX improvement for CLI users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add optional filename to PersistConfig
2 participants