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

Ci (compile and test in a GitHub action) #26

Conversation

JamesParrott
Copy link
Collaborator

@JamesParrott JamesParrott commented Feb 20, 2024

This PR provides a basic working Continuous Integration framework, that can run the tests on every commit.

It builds on the compile (#24) and testing (#22) ones, but may require a minor merge conflict with either to be resolved, it having refactored the test running part into a reusable Action, so that the compiler workflow can use the same code.

However one of the tests currently fails unpredictably (when sDNA does not reproduce the exact same ERRORs as in correctout.txt). So until that issue (#21) is resolved (either by understanding the reasons behind the behaviour, or tweaking the tests to accomodate it), it would cause lots of frustration to rely on this workflow to
break commits, and stop releases, due to the high percentage of non-useful test failures.

Therefore, for now, the on push and on pull_request triggers been commented out:
.github\workflows\compile_and_test.yml

on:
  workflow_dispatch:
  # push:
  #   branches: [ "main" ]

  # pull_request:
  #   branches: [ "main" ]

The Pytest docs have great advice on dealing with flakey tests https://docs.pytest.org/en/8.0.x/explanation/flaky.html.

Unfortunately for sDNA, none of the Pytest plug-ins suggested by support Python 2 (flaky, pytest-flakefinder, pytest-rerunfailures, and pytest-replay:).

…e_workflows_into_Actions

Ci refactor tests and compile workflows into actions
…eds_to_be_downloaded

The build files build the sDNA installer using the Geos binary in source control, so there's no point downloading one.
@JamesParrott
Copy link
Collaborator Author

JamesParrott commented Mar 9, 2024

In order to give confidence that the code in this PR is correct, in commit bc6def0 I have added a work around for issue #21 .

If the env variable ALLOW_NEGATIVE_FORMULA_ERROR_ON_ANY_LINK_PRESENT is Truthy (i.e. non-empty - don't use 0 as in Python bool("0") is True), extra code runs which allows lines in sDNA\sdna_vs2008\tests\correctout.txt to be skipped, only if:
a) both actual and expected lines start with: ERROR: Formula evaluation gave negative result for link n (in order to allow the expected n, and actual n to be diffferent). And:
b) in the actual output lines, the link numbers n in all error lines above, subequently occur in another line of the form: Polyline n id=i for some i (before the next negative formula error line, as above).

The tests pass locally in Python 3.12, and 2.7 using the command (in a venv with pytest and numpy installed):

set DONT_TEST_N_LINK_SUBSYSTEMS_ORDER=1 & set ALLOW_NEGATIVE_FORMULA_ERROR_ON_ANY_LINK_PRESENT=1 & pytest -rA

and reliably now in CI on Python 2.7 and on Pythons 3.5, 3.6, ..., 3.12 https://github.com/JamesParrott/sdna_plus/actions/runs/8215755710/job/22469718739

Filtering out this and any other strange behaviour from a testing strategy is sub-optimal. But they are only errors if the original test data (e.g. correctout.txt) is assumed to be correct or desired behaviour. The differences occur due to subtle reasons and are a lot of work to even understand, let alone fix.

Apart from these issues, 10s of thousands of other output lines are reproduced exactly, without being skipped. Therefore in the mean time, the benefits of automating the tests, and having a working (otherwise correct) non-flakey CI pipeline at all, are far more beneficial, than having no CI pipeline (or than having a CI pipeline with false negatives, if it is required to reproduce exactly the same errors, and the same output lines, but also in exactly the same order, as some previous build, in some other test environment did).

@JamesParrott JamesParrott merged commit 37d0b3a into fiftysevendegreesofrad:main Mar 9, 2024
10 checks passed
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.

1 participant