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: expand allowed removeKeyHash length from exactly 32 to 16-32 #248

Conversation

cascornelissen
Copy link
Contributor

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

Description

As discussed in #241.

@shellscape
Copy link
Owner

Change looks good, but we'll need a test added to approve it.

@cascornelissen cascornelissen force-pushed the expand-remove-hash-key-length-allowance branch from 9dcf5fe to 8101586 Compare February 6, 2021 09:27
@cascornelissen
Copy link
Contributor Author

cascornelissen commented Feb 6, 2021

Cool, I've added a test, don't have any experience with ava so let me know if something should be changed ✌🏼


No idea why the existing test would start failing on Node 10, maybe you know/can rerun the job?

@shellscape
Copy link
Owner

Click on the Details link on the failing check, and it'll take you to: https://github.com/shellscape/webpack-manifest-plugin/pull/248/checks?check_run_id=1844379311. Looks like your test on Windows is returning an unexpected value.

@cascornelissen
Copy link
Contributor Author

I got that part but it's failing on the existing test (which I haven't touched), not on the test I've added, unless I'm misinterpreting the output?

@cascornelissen
Copy link
Contributor Author

Ping @shellscape, sorry to bother you but I wanted to make clear that I need your help to finish this up 😅

@shellscape
Copy link
Owner

So the way the output reads, the test for the removeKeyHash is failing on windows. That's an option that you did modify, even if you didn't touch the tests for it. This is part of why we have unit tests like this in place - it catches regressions in seemingly innocent changes to parts of the code. Since you changed how the option works, you'll have to check that test on Windows (probably through console.log and looking at test results here) and see how/why your changes modified what that test expected.

@cascornelissen cascornelissen force-pushed the expand-remove-hash-key-length-allowance branch 3 times, most recently from 10efa33 to 3eeac03 Compare February 12, 2021 19:58
@codecov-io
Copy link

codecov-io commented Feb 12, 2021

Codecov Report

Merging #248 (a833d4f) into master (9f408f6) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #248   +/-   ##
=======================================
  Coverage   98.19%   98.19%           
=======================================
  Files           3        3           
  Lines         111      111           
=======================================
  Hits          109      109           
  Misses          2        2           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f408f6...3eeac03. Read the comment docs.

@cascornelissen cascornelissen force-pushed the expand-remove-hash-key-length-allowance branch from 3eeac03 to f589030 Compare February 12, 2021 20:03
@cascornelissen cascornelissen force-pushed the expand-remove-hash-key-length-allowance branch from f589030 to 301c595 Compare February 12, 2021 20:07
@cascornelissen
Copy link
Contributor Author

Apparently the test output was incorrect, deleting it and letting it regenerate solved the problem. Couldn't test it locally so had to use the GH action a bunch of times.

@shellscape
Copy link
Owner

bueno!
thanks!

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.

3 participants