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: reportTitle option (allow reproducible build) #354

Merged
merged 1 commit into from
Jun 8, 2020

Conversation

eoingroat
Copy link
Contributor

Allows setting the HTML report title

tests

Tests are included, they test string and function reportTitle set through the plugin API, and error propagation for the function reportTitle

why

I, and increasingly others, require reproducible builds with CI; this is a more specific PR targeting a single option after comments on a more general PR #349

@valscion
Copy link
Member

valscion commented Jun 1, 2020

Yeah this is looking much better, thanks!

It seems that the CLI currently requires the --title option and doesn't default to the previous title, right? Could we add a test for the CLI to make sure default title works? And maybe another test to test that the custom title also works.

I'm not sure if we have test coverage for the old title, either, so if you're in the mood, having some OK-level coverage for the existing title generation logic would be nice (i.e. if we can test the title generation without having to stub time, it would be nice — a regex-test on the title would also suffice)

@eoingroat
Copy link
Contributor Author

eoingroat commented Jun 1, 2020

Ah yes; that was fairly low quality of me to not include those tests (particularly as the default was became buggy).

I've gone for a very vague regex for the title, mainly to eliminate potentially stringified error values

@valscion
Copy link
Member

valscion commented Jun 2, 2020

Looking much better!

Now the default title is duplicated, and I wonder if we could de-duplicate and put it into utils. It seems that the getCurrentTime export is only used in generating the default title, so we can stop exporting getCurrentTime and instead move to export getDefaultReportTitle function which both the plugin-side and the CLI-side can use.

Also for the regex, I think we can be a bit more strict and hopefully make the regex a bit more clear. What do you think of using this regex?

expect(generatedReportTitle).to.match(/^webpack-bundle-analyzer \[.* at \d{2}:\d{2}\]/u);

I'm not sure if there's a situation where process.env.npm_package_name would not be defined as webpack-bundle-analyzer when running the tests.

If however we would not like to rely on webpack-bundle-analyzer to be there, we could also be even more relaxed in the regex so that it would not be so brittle yet it would make sure that the time of day is printed in proper format:

expect(generatedReportTitle).to.match(/^.+ \[.* at \d{2}:\d{2}\]/u);

@eoingroat eoingroat force-pushed the set-title-option branch 2 times, most recently from 9fbba1c to 56ea889 Compare June 5, 2020 10:03
Adds CLI options '-t' and '--title' taking strings
Adds api option 'reportTitle' taking strings and functions
Retains existing title behaviours as a default function
Updates README
Adds tests for plugin option reportTitle as string, function, errored function
  and default value
Adds tests for cli option title as string and default value
Copy link
Member

@valscion valscion left a comment

Choose a reason for hiding this comment

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

Splendid! Thank you ☺️

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.

2 participants