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

Keep only one directory for tests #17257

Merged
merged 5 commits into from
Jun 6, 2024

Conversation

trasher
Copy link
Contributor

@trasher trasher commented Jun 5, 2024

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes

The unit directory is run with an 10 children subprocesses; but that no longer have sense since there are very few tests in there; and execution time is the same when parameter is set to 1.

I think it's easier having all files in one directory only

@codecov-commenter
Copy link

codecov-commenter commented Jun 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.91%. Comparing base (cb39520) to head (9f8328d).
Report is 166 commits behind head on 10.0/bugfixes.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@                Coverage Diff                @@
##           10.0/bugfixes   #17257      +/-   ##
=================================================
+ Coverage          44.39%   44.91%   +0.52%     
=================================================
  Files                232      215      -17     
  Lines              37928    35943    -1985     
=================================================
- Hits               16837    16144     -693     
+ Misses             21091    19799    -1292     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@trasher
Copy link
Contributor Author

trasher commented Jun 6, 2024

I do not understand the fail on CronTask test; I do not reproduce on my instance :(

@cedric-anne
Copy link
Member

I do not understand the fail on CronTask test; I do not reproduce on my instance :(

This was due to the presence of files generated by other test methods. My last commit should fix it.

@trasher trasher merged commit a63aa66 into glpi-project:10.0/bugfixes Jun 6, 2024
14 checks passed
@trasher trasher deleted the feature/onedir_tests branch June 6, 2024 09:51
@AdrienClairembault
Copy link
Contributor

AdrienClairembault commented Jun 11, 2024

execution time is the same when parameter is set to 1

Are you sure about this ? On my machine the tests/units suite take 46s with mcn=10 and 106s with mcn=1.
That is 2.5 times slower, which is not great.

IMO we should be moving in the opposite direction by fixing our tests so that they can all run concurrently without issues.

@AdrienClairembault
Copy link
Contributor

Also, out of these 106 seconds of tests, only 20 seconds are used to run the actual tests content + before/after hooks.

I am curious where the others 86 seconds come from, if its just from the atoum framework that seems super excessive :/

@trasher
Copy link
Contributor Author

trasher commented Jun 12, 2024

For execution times; I check difference on github action, not on my local computer. Time was the same with mcn set to 1 (a few seconds less or more).
On GH actions, running tests on unit directory was taking less than 30 seconds - that's quite insignificant.

I do not consider that as something to be fixed: tests take time, we surely can improve several other parts of the core code or the process.

Also, atoum is dead and will be replaced with phpunit - problems and cause will differ.

anthonymontebrun pushed a commit to IT-Gouvernance/glpi that referenced this pull request Jul 11, 2024
* Keep only one directory for tests

* Fix namespaces

* Make the CronTask::testCronTemp() test more resilient

* Remove remaining usages of the tests/unit dir

* Remove unit from coverage, add codecov token

---------

Co-authored-by: Cédric Anne <cedric.anne@gmail.com>
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

4 participants