Simplify check hasDependencies and add unit tests #6346
Conversation
|
@Connormiha you could ignore the failure. |
|
Unit tests do not contribute to test coverage. To test this add an integration test which covers this scenario. |
|
@sokra |
lib/DependenciesBlockVariable.js
Outdated
| } else { | ||
| if(this.dependencies.length > 0) return true; | ||
| return this.dependencies.length > 0; |
There was a problem hiding this comment.
You can omit else block and just type this statement on the end of the function
webpack-bot
added
PR: review-outdated
PR: CI-not-ok
PR: CI-ok
and removed
PR: reviewed-approved
PR: CI-ok
PR: CI-not-ok
labels
|
@ooflorent should i do something else to avoid CI warnings at this PR( i think these were falsely warnings)? |
|
@Connormiha the warning is fine. I've removed it. EDIT: Grr. |
|
@Connormiha Thanks for your update. I labeled the Pull Request so reviewers will review it again. @sokra Please review the new changes. |
|
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
|
For maintainers only:
|
# Conflicts: # test/DependenciesBlockVariable.unittest.js
sokra
force-pushed
the
simplife-has-dependencies
branch
from
2f73628
to
dddb547
Compare
|
It looks like this Pull Request doesn't include enough test cases (based on Code Coverage analysis of the PR diff). A PR need to be covered by tests if you add a new feature (we want to make sure that your feature is working) or if you fix a bug (we want to make sure that we don't run into a regression in future). @Connormiha Please check if this is appliable to your PR and if you can add more test cases. Read the test readme for details how to write test cases. |
|
Thanks |
What kind of change does this PR introduce?
Added unit tests + minor refactoring
Did you add tests for your changes?
Yes
Summary
Added different test cases for
hasDependencies.Simplify mehod.
Does this PR introduce a breaking change?
No