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 flaky test_time_overhead_on_handlers_of_auth_decorators #5279

Merged
merged 2 commits into from
Jan 31, 2024

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Jan 29, 2024

What do these changes do?

test_time_overhead_on_handlers_of_auth_decorators would run sequentially two requests (w/ and w/o auth) and compare their times to make sure the overhead of auth_decorators was bound.

@sanderegg correctly pointed out that this test was not well defined since in the CI multiple tests are run in parallel and we cannot control how busy the machines are . Therefore even if in the original test we do a relative comparison, we cannot guarantee that each of the individual runs were done in "equivalent" conditions.

To resolve this issue, this test has been refactored to run multiple times each request and compute use the median of each of the runs do the comparison ( Note that the mean is strongly affected by outliers and is not necessarily a typical example of the data points. For a more robust, although less efficient, measure of central tendency we use media. Quote from 1)

NOTE: I also tried to use pytest-benchmark to do this test I had many troubles with the event loop (benchrmark needs a sync callable) and also was not clear to me how to compare the two different runs.

Related issue/s

How to test

cd services/web/server
make install-dev
pytest -vv --pdb tests/unit/isolated/test_security_api.py

Two driving tests:

  • test_number_of_db_calls_on_handlers_of_auth_decorators ensures max. number of calls to the db from decorators
  • test_time_overhead_on_handlers_of_auth_decorators benchmarks class w/ and w/o authz and compares time overhead.

Dev Checklist

DevOps

None

@pcrespov pcrespov self-assigned this Jan 29, 2024
@pcrespov pcrespov added the a:webserver issue related to the webserver service label Jan 29, 2024
@pcrespov pcrespov added this to the This is Sparta! milestone Jan 29, 2024
Copy link

codecov bot commented Jan 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (fc33561) 87.3% compared to head (35351ee) 76.8%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #5279      +/-   ##
=========================================
- Coverage    87.3%   76.8%   -10.5%     
=========================================
  Files        1316     542     -774     
  Lines       53813   27142   -26671     
  Branches     1170     202     -968     
=========================================
- Hits        46988   20857   -26131     
+ Misses       6576    6235     -341     
+ Partials      249      50     -199     
Flag Coverage Δ
integrationtests 60.8% <ø> (-4.4%) ⬇️
unittests 86.6% <ø> (+1.4%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 912 files with indirect coverage changes

@pcrespov pcrespov force-pushed the fix/flaky-test-login branch from 39da5e0 to 35351ee Compare January 30, 2024 20:35
@pcrespov pcrespov marked this pull request as ready for review January 30, 2024 20:35
@pcrespov pcrespov changed the title WIP:♻️ fixes flaky test_time_overhead_on_handlers_of_auth_decorators ♻️ fixes flaky test_time_overhead_on_handlers_of_auth_decorators Jan 30, 2024
Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@pcrespov pcrespov enabled auto-merge (squash) January 30, 2024 20:55
@pcrespov pcrespov merged commit 6706f0f into ITISFoundation:master Jan 31, 2024
55 checks passed
@pcrespov pcrespov deleted the fix/flaky-test-login branch February 2, 2024 16:53
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Feb 14, 2024
39 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:webserver issue related to the webserver service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants