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

Fixes some of the test #1330

Merged
merged 4 commits into from Dec 9, 2019
Merged

Fixes some of the test #1330

merged 4 commits into from Dec 9, 2019

Conversation

keshav234156
Copy link
Member

Fixes Some of the #1285

@keshav234156 keshav234156 changed the title Issue 1285 Fixes some of the test Dec 6, 2019
@codecov
Copy link

codecov bot commented Dec 6, 2019

Codecov Report

Merging #1330 into main will increase coverage by 34.44%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #1330       +/-   ##
===========================================
+ Coverage   32.13%   66.58%   +34.44%     
===========================================
  Files         108      125       +17     
  Lines        1982     2547      +565     
  Branches      292      397      +105     
===========================================
+ Hits          637     1696     +1059     
+ Misses       1345      851      -494
Impacted Files Coverage Δ
src/modules/Average/Module.js 100% <ø> (+63.63%) ⬆️
src/modules/GridOverlay/GridOverlay.js 89.47% <0%> (ø)
src/util/ParseInputCoordinates.js 73.33% <0%> (ø)
src/modules/_nomodule/gpuUtils.js 83.87% <0%> (ø)
src/modules/Threshold/Threshold.js 83.72% <0%> (ø)
src/modules/Dither/Dither.js 58.13% <0%> (ø)
src/modules/Crop/Crop.js 97.72% <0%> (ø)
src/modules/NoiseReduction/NoiseReduction.js 65.21% <0%> (ø)
src/modules/DrawRectangle/DrawRectangle.js 100% <0%> (ø)
src/modules/EdgeDetect/EdgeUtils.js 86.95% <0%> (ø)
... and 49 more

@keshav234156
Copy link
Member Author

Oh! I just saw the coverage report. Coverage would almost be doubled and at all time maxima. @jywarren We are on the right path but need to work a harder on commented test module that was preventing other modules

// test('Text Overlay module works correctly', function(t) {

Choose a reason for hiding this comment

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

Can we use multi line comments here? /* */ Just a suggestion

@keshav234156
Copy link
Member Author

@jywarren I think we should merge this so that our other test are not affected.what your view on this!!!

@jywarren
Copy link
Member

jywarren commented Dec 9, 2019

Yes, ok let's merge this as soon as it passes!

@jywarren jywarren merged commit 032971d into publiclab:main Dec 9, 2019
@jywarren
Copy link
Member

jywarren commented Dec 9, 2019

Can you make the PR title a little more descriptive? It's good to leave a more thorough history! Thanks a lot!!!

@keshav234156
Copy link
Member Author

@jywarren our next step should be fix 7 modules that i have mentioned in #1285 .For that i found the module were changed as it has some bugs like the rotate module was earlier having fixed dimension but corrosponding benchmarks were not changed as a result they are failing.So for this we can change the benchmarks of that modules.Then our next step should be if there is any failure in test then travis should raise an error rather than passing it .What your view 's regarding this?

@keshav234156
Copy link
Member Author

@jywarren Would take care of descriptive title from now on.Thanks!!!

@jywarren
Copy link
Member

jywarren commented Dec 9, 2019

Thank you, that's a fantastic plan. And please copy your comment back into #1285 so we can continue tracking progress in one place -- thanks a ton!!!

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.

None yet

3 participants