Introduce 'long_test' keyword in UTscapy

[polybassa]

This PR does two things.

First: It enforces that every test with an execution time longer than 10s need to be marked with the keyword long_test. Second: Tests with the keyword long_test are only executed with a probability of 10% to save CI credits

[gpotter2]

It's a pretty good idea to add a timeout to tests, that'll help a lot.

However, I'd say either 10sec is too low, either we have some more tests to mark, either we need to add this tag in other places. Some tests (e.g. ICMP) are network dependent and use retry_test (to retry until they work).

It's nice to save credits. I need to check if we can't continue migrating to Travis CI though, there aren't many things left over there.

[polybassa]

I thought about that as well. I suggest to mark all tests which require more than 10 seconds with that keyword. This helps to identify tests which might need a refactoring and ensures that no new tests with long runtimes come in. I would say, for most tests it doesn't harm if they are only executed once a while. If you run a local unit test on your system, by default all tests will get executed.

On January 8, 2021 7:41:26 PM GMT+01:00, Gabriel [email protected] wrote:

It's a pretty good idea to add a timeout to tests, that'll help a lot.

However, I'd say either 10sec is too low, either we have some more tests to mark, either we need to add this tag in other places. Some tests (e.g. ICMP) are network dependent and use retry_test (to retry until they work).

-- You are receiving this because you modified the open/close state. Reply to this email directly or view it on GitHub: https://github.com/secdev/scapy/pull/3043#issuecomment-756929824

[polybassa]

To summarize my idea: If a test can't finish in less than 10s, it doesn't deserve to be executed every time.

[polybassa]

The downside of this changes might be a instability in codecov. But still, I would prefer to execute some tests once a while instead of disabling them. This gives us at least a feedback in a reasonable time, if some commit broke one of the tests marked as 'long_test'.

[codecov[bot]]

Codecov Report

Merging #3043 (ab6f1f7) into master (cf0c4a1) will increase coverage by 0.16%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3043      +/-   ##
==========================================
+ Coverage   85.32%   85.49%   +0.16%     
==========================================
  Files         256      256              
  Lines       53997    54004       +7     
==========================================
+ Hits        46075    46170      +95     
+ Misses       7922     7834      -88     

Impacted Files	Coverage Δ
scapy/scapypipes.py	75.47% <0.00%> (-7.87%)	:arrow_down:
scapy/contrib/automotive/uds.py	90.42% <0.00%> (-0.88%)	:arrow_down:
scapy/contrib/automotive/gm/gmlanutils.py	88.26% <0.00%> (-0.50%)	:arrow_down:
scapy/utils.py	78.51% <0.00%> (-0.40%)	:arrow_down:
scapy/fields.py	91.20% <0.00%> (-0.29%)	:arrow_down:
scapy/pipetool.py	85.59% <0.00%> (-0.21%)	:arrow_down:
scapy/contrib/pfcp.py	98.54% <0.00%> (-0.13%)	:arrow_down:
scapy/packet.py	82.13% <0.00%> (-0.08%)	:arrow_down:
scapy/arch/windows/__init__.py	68.25% <0.00%> (+0.38%)	:arrow_up:
scapy/layers/l2.py	76.17% <0.00%> (+0.70%)	:arrow_up:
... and 2 more

[gpotter2]

I think it would make more sense if we just had a flag to disable long tests, and we would enable it on all runs but one (for instance, the ubuntu 3.9 one). I'm not a huge fan of the random factor

[polybassa]

No problem. I can change that. Do you think the enforcement of the "long_test" keyword is a beneficial addition?

[gpotter2]

I'm unsure, what do you think @p-l- @guedou ?

[p-l-]

I'm not sure. Both have advantages. With random, you get more chance to find corner-case bugs. Without, you get stable, predictable CI. Both have their benefits and drawbacks. (I understand that does not help you chose, sorry about that!)

[polybassa]

Closed because of inactivity

[polybassa]

and its useless ;-D