search

shellscape / webpack-manifest-plugin

webpack plugin for generating asset manifests
MIT License
1.44k stars 184 forks source link

feat: expand allowed removeKeyHash length from exactly 32 to 16-32 #248

Closed cascornelissen closed 3 years ago

cascornelissen commented 3 years ago

This PR contains:

Are tests included?

Breaking Changes?

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 https://github.com/shellscape/webpack-manifest-plugin/issues/241.

shellscape commented 3 years ago

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

cascornelissen commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

shellscape commented 3 years ago

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.

codecov-io commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

bueno! thanks!