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

Null safety migration #39

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

civts
Copy link

@civts civts commented Mar 10, 2021

This pull request removes discontinued lcov dependency and migrates the package to null safety.

Following Flutter guidelines, package version gets updated from 0.5.0 to 0.6.0

Closes #38 , #37, #33

@bradyt
Copy link

bradyt commented Mar 14, 2021

Hmm, if I use 0.4.2, I can still see sub-100 coverage, like Overall line coverage rate: 94%., but if I checkout this branch, it just reads as 100%. Something will break --min-coverage usecase?

@civts
Copy link
Author

civts commented Mar 14, 2021

Does that behavior appear also if you checkout the master branch?
I am asking this because since you are referring to version 0.4.2 but master is at 0.5.0, the issue may be unrelated to this PR.

@bradyt
Copy link

bradyt commented Mar 14, 2021

Yes, at commit 2bfaa88b9aa1fd0a3a921e6ead9ae59f54920224, I see Overall line coverage rate: 99%..

@civts
Copy link
Author

civts commented Mar 14, 2021

Ok, then I think creating a new issue may be the best way to go to keep track of everything in a dedicated thread.
Let me know what you think about that.

@bradyt
Copy link

bradyt commented Mar 15, 2021

Apologies, did I miscommunicate? The commit, 4bc829607f5aef3562528478c6aec39f8d1a4480, introduces the problem.

Are you still suggesting to file a new issue?

You can try this diff:

modified   test/stub_package/lib/stub_package.dart
@@ -1,3 +1,7 @@
 int add(int a, int b) {
   return a + b;
 }
+
+int subtract(int a, int b) {
+  return a - b;
+}

Then run dart run test_coverage --min-coverage=90 --no-badge. It correctly fails at 2bfaa88b9aa1fd0a3a921e6ead9ae59f54920224, since 50 is less than 90. But at 4bc829607f5aef3562528478c6aec39f8d1a4480, it incorrectly succeeds, with 100 >= 90.

@civts
Copy link
Author

civts commented Mar 15, 2021

Thank you very much for the additional information. I think I have understood now 👍

I can confirm that there was a problem with overall line coverage in commit 4bc829607f5aef3562528478c6aec39f8d1a4480. It should now be fixed.

I manually tested dart run test_coverage in the following cases:

  1. when total coverage is below the --min-coverage threshold
  2. when a test fails
  3. when all tests succeed and coverage is >= than the threshold

All three cases produced the expected result (respectively failed, failed, succeeded)

You should not be experiencing the issue anymore. Can you verify?

@bradyt
Copy link

bradyt commented Mar 15, 2021

Looks great!

% dart run test_coverage --no-badge --min-coverage=90
Found 2 test files.
Generated test-all script in test/.test_coverage.dart. Please make sure it is added to .gitignore.
Coverage report saved to "coverage/lcov.info".
Overall line coverage rate: 50%.
Overall coverage 50 is less than minimum required coverage 90
% echo $?
1

@QuirijnGB
Copy link

@pulyaevskiy Can we get this merged? or give some more direction if it needs changes.

@QuirijnGB
Copy link

@civts Would you be keen to publish a fork?

@jpnurmi
Copy link

jpnurmi commented Apr 7, 2021

I haven't looked closer what was changed but there's already one fork: https://pub.dev/packages/test_cov

@civts
Copy link
Author

civts commented Apr 9, 2021

@jpnurmi from what I can see https://pub.dev/packages/test_cov diverged quite a bit from this implementation.
Even though it is indeed null-safe, --min-coverage option and badge generation do not appear to be supported there.

Looking around, I found another package, https://github.com/f3ath/check-coverage, which is meant to only check when the coverage is below a given threshold, without generating the report.

Given that, @QuirijnGB, I am thinking about making a new package from scratch that works similar to this one but does not require to generate the intemediate .test_coverage.dart file.
Let me know if have any thoughts on the idea and/or would like to contribute 👍🏻
https://github.com/civts/gen-coverage

@jpnurmi
Copy link

jpnurmi commented Apr 10, 2021

@civts Oh, ok. I'm using Codecov + their ready-made GitHub Actions so all I needed was lcov.info. For me, test_cov was a drop-in replacement for test_coverage. Badges and options are provided by Codecov.

@civts
Copy link
Author

civts commented Apr 10, 2021

@jpnurmi For purely generating the lcov.info file, you may also consider making your package depend directly on test and coverage and generating the file with a script like the one below.

#Run tests and collect coverage info
dart run test --coverage .tempCoverageDir
#Generate lcov.info file in ./coverage
dart run coverage:format_coverage -l -c --report-on lib -i .tempCoverageDir --packages .packages -o coverage/lcov.info
#Clean up unneeded coverage info
rm -r .tempCoverageDir

Worth noting that both these packages are maintained by dart-lang.

@jpnurmi
Copy link

jpnurmi commented Apr 10, 2021

Or just run dart run test_cov and be done with it 😏

@civts
Copy link
Author

civts commented Apr 11, 2021

Yep. I myself would prefer to have a package that takes care of everything for me.
On the other hand, I think those commands may be quite useful for making the CI pipelines more robust, so I included them anyway.

As for test_cov, I tried to run it in one project and it freezed 😅

pY4x3g pushed a commit to redPanda-project/redpanda_light_client that referenced this pull request Aug 17, 2021
@pY4x3g
Copy link

pY4x3g commented Aug 17, 2021

Thanks, this pull request did the trick for me, the old code is no longer compatible with moor. The test_cov package did also not work for me. I enforced the pull request with:

test_coverage:
   git:
     url: https://github.com/civts/test-coverage.git
     ref: aa6ffb41c28d7dec4e3bb11c3b7c4729e8271a69

in the pubspec.yaml

Edit: hopefully this PR gets merged soon.

@pY4x3g pY4x3g mentioned this pull request Aug 17, 2021
@santitigaga
Copy link

why is taking so long to approve this pull request?

@civts
Copy link
Author

civts commented Aug 19, 2021

I do not know if this repository is being maintained anymore.
While I was waiting, I created my own package for generating the lcov.info file and enforcing minimum coverage.
If you want to give it a try, fork it or create a PR, this is the link https://github.com/civts/gen-coverage

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.

Remove lcov dependency
6 participants