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

core(index): update api helpers to use FR #14011

Merged
merged 11 commits into from
Jun 22, 2022
Merged

Conversation

adamraine
Copy link
Member

@adamraine adamraine commented May 16, 2022

Follow up to #13783.

@adamraine adamraine requested a review from a team as a code owner May 16, 2022 23:05
@adamraine adamraine requested review from brendankenny and removed request for a team May 16, 2022 23:05
@brendankenny
Copy link
Member

brendankenny commented May 16, 2022

See the getPrintString tests as well. The last one in particular is important to ensure the printed config is usable as a config.

@@ -82,8 +82,46 @@ describe('CLI Tests', function() {
});

describe('print-config', () => {
describe('fraggle rock', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

I created a "fraggle rock" block instead of a "legacy" block so the snapshots would be preserved.

@@ -82,8 +82,46 @@ describe('CLI Tests', function() {
});

describe('print-config', () => {
describe('fraggle rock', () => {
it('should print the default config and exit immediately after', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Oh man, I didn't realize this snapshot was from a --print-config test. I vote we don't do this one and delete the matching legacy "should print the default config and exit immediately after" and rid ourselves of some of the annoying -u churn in here.

The "overridden config" test below covers the cli flag sufficiently and the print string unit tests are where the real testing is. I feel like this one is just causing snapshot blindness due to the constant updates it needs.

Copy link
Member Author

@adamraine adamraine May 17, 2022

Choose a reason for hiding this comment

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

FWIW this test helped me catch an issue where artifact dependencies were showing up in the printed config even though dependencies aren't declared in the config json.

That condition can be added to a more specific test though so I could go either way.

Copy link
Member

Choose a reason for hiding this comment

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

That condition can be added to a more specific test though so I could go either way.

Maybe could do --only-audits charset instead of metrics in the test below this? That should require MainDocumentContent which depends on DevtoolsLog.

@devtools-bot devtools-bot deleted the node-api-helpers-fr branch June 22, 2022 21:49
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.

4 participants