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

chore(utils): add getPaths helper as separate util #674

Merged
merged 15 commits into from
Jul 9, 2020

Conversation

knagaitsev
Copy link
Contributor

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

The getPaths helper should be exported so that it can be used in webpack-dev-server: webpack/webpack-dev-server#2671

Still need to add tests

Breaking Changes

None

Additional Info

@codecov
Copy link

codecov bot commented Jul 6, 2020

Codecov Report

Merging #674 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #674   +/-   ##
=======================================
  Coverage   99.11%   99.12%           
=======================================
  Files           8        9    +1     
  Lines         227      228    +1     
  Branches       70       70           
=======================================
+ Hits          225      226    +1     
  Misses          2        2           
Impacted Files Coverage Δ
src/utils/getFilenameFromUrl.js 97.29% <100.00%> (-0.67%) ⬇️
src/utils/getPaths.js 100.00% <100.00%> (ø)

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 9278e5c...54dfa56. Read the comment docs.

alexander-akait
alexander-akait previously approved these changes Jul 6, 2020
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

/cc @hiroppy

@hiroppy
Copy link
Member

hiroppy commented Jul 7, 2020

@Loonride This PR is WIP?

Oh, I see. Need to add tests.

@knagaitsev knagaitsev force-pushed the refactor/get-paths branch from 94a6fa8 to ac17350 Compare July 7, 2020 21:12
@alexander-akait
Copy link
Member

@Loonride Still broken 😞

// the compilation needs to finish, as it will still be running
// after the test is done if not finished, potentially impacting other tests
compiler.hooks.done.tap('wdm-test', () => {
done();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't reproduce the problem locally, but there was a problem with api.test.js impacting getFilenameFromUrl.test.js when it was being run directly after api.test.js, happening on CI for webpack@next only.

The problem was that the very last test of api.test.js was not waiting for compilation to finish, and somehow this was impacting the very first test of getFilenameFromUrl.test.js by preventing the middleware's done hook from being called for this very first test.

I'm not sure why this was happening, but it could be something with webpack@next saving some global data about compiler hooks that is still there when the new webpack compilation starts. Also still not sure why I couldn't reproduce locally, I tried to make everything identical to the CI environment and installed webpack@next but tests still passed.

Copy link
Member

Choose a reason for hiding this comment

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

Very strange, if I comment this line, it is happens in CI, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@knagaitsev knagaitsev changed the title WIP: add getPaths helper as separate util chore(utils): add getPaths helper as separate util Jul 8, 2020
@knagaitsev
Copy link
Contributor Author

knagaitsev commented Jul 9, 2020

/cc @evilebottnawi @hiroppy

Sorry if util tests are slightly repetitive to the middleware and api tests, but I don't like making use of a util in webpack-dev-server if that util isn't directly tested

If it's fine with everyone, I can add tests for more of the utils files - I will do this in a different PR

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

All fine, we can merge it, many tests are always good

/cc @hiroppy

@hiroppy hiroppy merged commit 6afd559 into webpack:master Jul 9, 2020
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