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

t.rast.algebra/testsuite: split file test_raster_algebra.py #2974

Merged
merged 2 commits into from
Jun 3, 2023

Conversation

neteler
Copy link
Member

@neteler neteler commented May 20, 2023

In order to address the frequent CI timeout during execution of the test_raster_algebra.py file this file is split into two parts with roughly similar test duration.

In order to address the frequent CI timeout during execution of the `test_raster_algebra.py` file this file is split into two parts with roughly similar test duration.
@neteler neteler added backport_needed CI Continuous integration Python Related code is in Python labels May 20, 2023
@neteler neteler added this to the 8.4.0 milestone May 20, 2023
@wenzeslaus
Copy link
Member

Alternatively, we could increase the time out if you think it is really this one test hitting the time limit as opposed to hanging forever. The limit was set in 89fe85c (#2172).

While the temporal timeouts are a long-term issue (#2185), the recent flood of failures is due to various network issues (OSGeo for sample data and others for dependencies). A series of failures is also due to g.extension like this run on main for #2966 which timed out and gave:

Running ./scripts/g.extension/testsuite/test_addons_download.py...
========================================================================
WARNING: Bug in UI description. Missing module description
.......========================================================================
FAILED ./scripts/g.extension/testsuite/test_addons_download.py - Timeout >300.0s

This run for #2955 fails with just like the first attempt of runs for this PR:

Running ./temporal/t.rast.what/testsuite/test_what.py...
========================================================================
..............========================================================================
FAILED ./temporal/t.rast.what/testsuite/test_what.py - Timeout >300.0s

In grass-addons, the CI jobs are often canceled by GitHub automation after exceeding 6 hours with the following as the last line (like this one for OSGeo/grass-addons#606):

Running ./temporal/t.rast.what.aggr/testsuite/test_whataggr.py...
Error: The operation was canceled. 

@ninsbl
Copy link
Member

ninsbl commented May 21, 2023

If you run the t.rast.algebra test locally, it clocks in just around the timeout value. So, for this particular test, increasing the timeout threshold will help...

@neteler
Copy link
Member Author

neteler commented May 22, 2023

If you run the t.rast.algebra test locally, it clocks in just around the timeout value.

Confirmed also here, that's why I have split it (thematically) into two files with runtime of ~ 50% timeout each.
Personally I prefer shorter files anyway.

So, for this particular test, increasing the timeout threshold will help...

Since the timeout now happened elsewhere, we might consider that in addition.

@wenzeslaus
Copy link
Member

Timing

These are times coming directly from the test report generated in the CI.

The main branch

Test test_raster_algebra
Testsuite ./temporal/t.rast.algebra
Test file ./temporal/t.rast.algebra/testsuite/test_raster_algebra.py
Status succeeded
Return code 0
Number of tests 43
Successful tests 43
Failed tests 0
Percent successful 100%
Test duration 0:03:48.910987
Tested modules t.rast.algebra

This PR

Test test_raster_algebra_part1
Testsuite ./temporal/t.rast.algebra
Test file ./temporal/t.rast.algebra/testsuite/test_raster_algebra_part1.py
Status succeeded
Return code 0
Number of tests 20
Successful tests 20
Failed tests 0
Percent successful 100%
Test duration 0:02:41.177489
Tested modules t.rast.algebra
Test test_raster_algebra_part2
Testsuite ./temporal/t.rast.algebra
Test file ./temporal/t.rast.algebra/testsuite/test_raster_algebra_part2.py
Status succeeded
Return code 0
Number of tests 23
Successful tests 23
Failed tests 0
Percent successful 100%
Test duration 0:02:42.036689
Tested modules t.rast.algebra

Total: 5 min 23 sec (setup happens once for each test)

@wenzeslaus
Copy link
Member

...I have split it (thematically) into two files...Personally I prefer shorter files anyway.

Since you were able to split them thematically can you please name them that way. Splitting the long file thematically is actually best regardless of the times.

@neteler
Copy link
Member Author

neteler commented May 30, 2023

Files renamed:

  • test_raster_algebra_part1.py --> test_raster_algebra_arithmetic.py
  • test_raster_algebra_part2.py --> test_raster_algebra_operators.py

Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't really evaluate the names, but the names and shorter files are nice. Let's merge it.

@neteler neteler merged commit 19ab2f3 into OSGeo:main Jun 3, 2023
@neteler neteler deleted the tests_split_test_raster_algebra_file branch June 3, 2023 12:34
neteler added a commit that referenced this pull request Jun 3, 2023
In order to address the frequent CI timeout during execution of the `test_raster_algebra.py` file this file is split into two parts with roughly similar test duration.

Also rename split files.
@neteler neteler modified the milestones: 8.4.0, 8.3.0 Jun 3, 2023
neteler added a commit to nilason/grass that referenced this pull request Nov 7, 2023
In order to address the frequent CI timeout during execution of the `test_raster_algebra.py` file this file is split into two parts with roughly similar test duration.

Also rename split files.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous integration Python Related code is in Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants