Improve tests code even more #432
Conversation
The current, old API, is not only inconvenient (using a thread and a timer to implement timeouts) but it is also incorrect since the child process is not waited. Use subprocess.run for both `doShell` and `do`. Note that for the `do` method, this is a behavior change, since a child process is always terminated by kill. Add a default timeout, set to 10 min. After this change the test `system_mode_can_record_and_report_binary` fails. This will be addressed in the next commit.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #432 +/- ##
==========================================
- Coverage 65.70% 65.68% -0.03%
==========================================
Files 58 58
Lines 4514 4514
Branches 4171 4171
==========================================
- Hits 2966 2965 -1
- Misses 1548 1549 +1 ☔ View full report in Codecov by Sentry. |
|
As I suspected The strange thing is that |
|
Thanks! As for system-mode, it has really never been properly working anyway. It's sort of meant to be run as root, since the purpose is to collect coverage from all binaries in a system, as a part of the startup of an embedded system. However, it really was never working outside of a proof-of-concept stage, so I think we can simply disable the system mode tests for now. |
|
Thanks for the explanation. However it is true that before the change introducing Do you have a preferred channel for communication, like email, forum or github issue? |
In some rare case, shutil.rmtree from KcovTestCase.tearDown may fail, in case a child process continue to write additionally files. I can reproduce the behavior consistentily by setting default_timeout to 1 second. Update the tearDown method to retry rmtree in case of a ENOTEMPTY error, waiting 5 seconds to ensure that the offending child process terminates.
|
I'm fine with pr communication! What I mean is mainly that we can just remove the system mode tests. Maybe also the build of the binary, since it's not really working other than as a toy. |
A few tests use print to show a custom message, but the problem is that this will break the test status line. Add the KcovTestCase.write_message method, so that the message is showed correctly in the test status line.
The old name is long and not concise. As an example: parse_cobertura.parseFile(path) parse_cobertura.parse(data)
The system_mode_can_record_and_report_binary test tries to create the outdir directory, but it is already created by TestCase.setUp.
All the test case names use the snake case convention, except the basic module that uses the camel case convention. Make naming convention consistent.
Update the testcase.KcovTestCase.setUp method to add the configuration values as class members instead of globals. These global variables are still needed, but are hidden from the test modules. Remove the global kcov_system_daemon variable, since it is no longer necessary. Update all the test cases and reformat the code with `ruff format`.
The new function is meant to be used when running a child process that does not run kcov, both directly or indirectly. The method call usually returns noKcovRv, instead of rv. The reason for this change is to make these calls easier to detect, also avoiding having to set the kcovKcov parameter to False.
|
Good stuff! The tests feel much more proper now, and not like the plain hacks that they were before :-) Anyway, I've looked through your changes, and I think they look good! Tell me when you're ready and I'll merge them! |
The PR is ready for merge. However I would like to update the noKcovRv, o = self.do(self.sources + "/tests/python/main 5")I avoided the change since I was not sure of it was a oversight to not set Additionally, I found that in this pattern noKcovRv, o = self.doCmd(self.testbuild + "/executable")
rv, o = self.do(self.kcov + " " + self.outbase + "/kcov " + self.testbuild + "/executable", False)a few tests don't set I checked the code using > grep -A15 noKcovRv tests/tools/*.py` |
|
I forgot to add a commit. Fixed a typo in the commit message. |
Update the KcovTestCase.do method signature, so that kcovKcov is both a positional or keyword parameter, and the timeout is a keyword only parameter. I was planning to make kcovKcov a positional only parameter, but the `bash_linux_only.bash_exit_before_child` test used it as a keyword parameter. This change codifies the current usage.
perillo
force-pushed
the
improve-tests-code-even-more
branch
from
6a387eb to
9794dd0
Compare
|
Big thanks! In general, if I remember correctly, the kcovKcov stuff was to avoid running kcov on itself when collecting data for compiled code, where it's not possible to trace kcov itself since it would mean recursive ptrace, which is illegal. Thus, coverage data on kcov itself can be collected when covering bash and python, but not for compiled code. |
Additional improvements to
kcovtest suite.This PR changes a lot of code! I used
sedto make the change automatic.CHANGES
tests: use subprocess.run to run child process
The current, old API, is not only inconvenient (using a thread and a timer to implement timeouts) but it is also incorrect since the child process is not waited.
Use subprocess.run for both
doShellanddo. Note that for thedomethod, this is a behavior change, since a child process is always terminated by kill.Add a default timeout, set to 10 min.
After this change the test
system_mode_can_record_and_report_binaryfails. This will be addressed in the next commit.tests: make
shutil.rmtreemore robustIn some rare case
shutil.rmtreefromKcovTestCase.tearDownmay fails, incase a child process continue to write additionally files.
I can reproduce the behavior consistently by setting default_timeout to
1 second.
Update the
tearDownmethod to retryrmtreein case of aENOTEMPTYerror,waiting 5 seconds to ensure that the offending child process terminates.
tests: improve custom messages in a test case
A few tests use print to show a custom message, but the problem is that
this will break the test status line.
Add the
KcovTestCase.write_messagemethod, so that the message is showedcorrectly in the test status line.
tests: rename the
parse_coberturamodule tocoberturaThe old name is long and not concise.
As an example:
parse_cobertura.parseFile(path)parse_cobertura.parse(data)tests: remove unnecessary creation of outdir
The
system_mode_can_record_and_report_binarytest tries to create theoutdir directory, but it is already created by
TestCase.setUp.tests: use consistent naming convention in test cases
All the test case names use the snake case convention, except the basic
module that uses the camel case convention.
Make naming convention consistent.
tests: make configuration values
KcovTestCasemembersUpdate the
testcase.KcovTestCase.setUpmethod to add the configurationvalues as class members instead of globals.
These global variables are still needed, but are hidden from the test
modules. Remove the global
kcov_system_daemonvariable, since it is nolonger necessary.
Update all the test cases and reformat the code with
ruff format.tests: add
testbase.KcovTestCase.doCmdmethodThe new function is meant to be used when running a child process that
does not run
kcov, both directly or indirectly. The method call usuallyreturns
noKcovRv, instead ofrv.The reason for this change is to make these calls easier to detect, also
avoiding having to set the
kcovKcovparameter toFalse.tests: make calls to
KcovTestCase.domore strictUpdate the
KcovTestCase.domethod signature, so thatkcovKcovis both a positional or keyword parameter, and the timeout is a keyword
only parameter.
I was planning to make
kcovKcova positional only parameter, but thebash_linux_only.bash_exit_before_childtest used it as a keywordparameter.
This change codifies the current usage.
TODO
Additionally tests that should use
doCmd:system_mode.system_mode_can_start_and_stop_daemonsystem_mode.system_mode_can_record_and_report_binaryPossible BUG(?)
basic.lookup_binary_in_pathshould setkcovKcovtoFalse