Testing new way to specify azure tests #20060

markcmiller86 · 2024-11-20T05:57:16Z

Description

This PR adjusts how Azure CI testing is handled.

To add any .py file in src/test/tests to Azure CI, just add the string prci to the comma separated CLASSES list in the .py file header comments. Be sure to follow requirements specified in src/test/prci_checksum.txt though.
A new GitHub action computes a checksum over all such .py files and holds it in src/test/prci_checksum.txt
Azure CI is triggered when, among other paths, src/test/prci_checksum.txt has changed.

Type of change

Bug fix~~
New feature~~
Documentation update~~
Other~~

How Has This Been Tested?

Reminders:

Please follow the style guidelines of this project.
Please perform a self-review of your code before submitting a PR and asking others to review it.
Please assign reviewers (see VisIt's PR procedures for more information).

Checklist:

I have commented my code where applicable.~~
I have updated the release notes.~~
I have made corresponding changes to the documentation.~~
I have added debugging support to my changes.~~
I have added tests that prove my fix is effective or that my feature works.~~
I have confirmed new and existing unit tests pass locally with my changes.~~
I have added new baselines for any new tests to the repo.~~
I have NOT made any changes to protocol or public interfaces in an RC branch.~~

cyrush · 2024-11-20T20:53:21Z

I like this idea. One thing to check with the trigger: We need to make sure it triggers if the contents of symlink change. The symlink itself is a version controlled entity, so I am not 100% sure changes to the destination will be seen.

markcmiller86 · 2024-11-20T22:36:27Z

I like this idea. One thing to check with the trigger: We eed to make sure it triggers if the contents of symlink change. The symlink itself is a version controlled entity, so I am not 100% sure changes to the destination will be seen.

Doh! OMG...you are right. ChatGPT says the paths: clause will consider only changes to the symlink itself, not the target. Well, that is less than desireable 😢. I mean, we can probably be ok with a paths: entry of the form src/test/tests/*/*.py which will be a bit overkill but would still at least cover the tests actually run during Azure CI.

markcmiller86 · 2024-11-21T02:19:19Z

Ok, so I did get a pass with this approach in baee914. This approach makes it easier to add tests to Azure CI without having to change the azure-piplines.yml file but doesn't allow for a better trigger based on paths due to reasons mentioned above.

I am trying a different approach now.

…sit-dav/visit into task-mcm86-19nov24-expand-azure-tests

JustinPrivitera · 2024-11-21T18:02:54Z

src/test/tests/databases/blueprint.py

@@ -1,5 +1,5 @@
 # ----------------------------------------------------------------------------
-#  CLASSES: nightly
+#  CLASSES: nightly, prci


Your first condition for PRCI tests isn't true for blueprint: The test typically takes less than 20 seconds to run "normally".

The condition is about adding new tests...not about anything preexisting this work.

JustinPrivitera · 2024-11-21T18:03:55Z

I feel like we could use some dev docs explaining how to go about adding new tests to the CI. If you haven't seen this PR, the process is mysterious.

cyrush · 2024-11-21T19:03:11Z

@markcmiller86 this is growing a bit complex.

I think simply triggering CI if any py file changes is not the end of the world.

Don't really like the idea of extra commits from the robot to updated the checksum. (those will be in our history)

Also, worried about the timing interaction between github actions and azure.

I really don't like the fact that the CI could trigger on a later, unrelated PR.

markcmiller86 · 2024-11-21T21:12:28Z

@markcmiller86 this is growing a bit complex.

How is this more complex than checking the BLT hash as part of something called the "BLT Time Travel Check"?

I think simply triggering CI if any py file changes is not the end of the world.

Triggering CI is not really what this PR is about. I mean, its certainly part of it but a substantially smaller part than...

Being able to easily add (and remove) a .py file to the set of tests run during Pull Request CI without having to go change azure-pipelines.yml. With this PR, this is accomplished simply by adding (or removing) prci in the .py file header.
Having to maintain a wholly separate list of .py files to be used in Pull Request CI in azure-pipelines.yml.
Not triggering CI when changes don't change what CI computes.

Don't really like the idea of extra commits from the robot to updated the checksum. (those will be in our history)

But those are apparently ok from LC cron jobs during from nightly testing

Also, worried about the timing interaction between github actions and azure.

Nothing to worry about. As explained in prci_checksum.txt, if Azure fires first, it will be cancelled thereafter by the GitHub action.

I really don't like the fact that the CI could trigger on a later, unrelated PR.

Either I don't understand this statement or the statement is false.

JustinPrivitera · 2024-11-21T21:25:17Z

Triggering CI is not really what this PR is about. I mean, its certainly part of it but a substantially smaller part than...
* Being able to _easily_ add (and remove) a `.py` file to the set of tests run during Pull Request CI without having to go change `azure-pipelines.yml`. With this PR, this is accomplished simply by adding (or removing) `prci` in the `.py` file header.

you also have to add a symlink for it too

* Having to maintain a wholly separate list of `.py` files to be used in Pull Request CI in `azure-pipelines.yml`.

Either the list is there or it is in this new tests/azure folder. I think I prefer it in azure-pipelines.yml.

* Not triggering CI when changes don't change what CI computes.
Don't really like the idea of extra commits from the robot to updated the checksum. (those will be in our history)

But those are apparently ok from LC cron jobs during from nightly testing

I don't know that we want even more robo commits.

I agree with Cyrus that this all feels complicated. The barrier to entry to understanding how all this works is higher than before.

markcmiller86 · 2024-11-21T21:32:18Z

you also have to add a symlink for it too

Thats not true. Whoops...forgot to remove that stuff. Not needed anymore in this PR

JustinPrivitera · 2024-11-21T21:39:41Z

azure-pipelines.yml

@@ -158,8 +126,8 @@ stages:
           # add to ld_lib path (rpaths are missing?)
           export LD_LIBRARY_PATH=${VTK_LIB_DIR}:${OSPRAY_LIB_DIR}:${QT_LIB_DIR}
           # run test suite on silo + blueprint tests
-           export TESTS="$(python_test_files)"
-           ./run_visit_test_suite.sh --fuzzy --pixdiff 10 --avgdiff 10 --numdiff 0.1 -v ${TESTS}
+           export TESTS="$(find ../../src/test/tests -name '*.py' -exec grep -H -m 1 CLASSES {} \; | grep prci | cut -d':' -f1)"


I am not familiar enough with all of these commands to know exactly what is happening... if someone names a variable in a python test prci will the test get unwittingly triggered in CI?

markcmiller86 · 2024-11-21T21:41:09Z

I don't know that we want even more robo commits.

That seems to fly in the face of "more automation" to make our lives easier. Something I've also heard discussed a lot in our team recently.

I don't know what to do about vague statements such as "this feels complex".

I can respond to specific issues for which a technical argument has been offered.

Ok, I see another email...feels like people are just looking for reasons to decline this. I will close.

cyrush · 2024-11-22T00:29:49Z

It looks like this could create a commit for every push to a PR, which is not the same as the test suite results (max of one commit per day to the develop branch).

Are those pushed to the current branch? Will those re-trigger the CI? I don't understand it and can't predict how the multiple CI jobs and commits will interact.

Could we still have the symlink folder to organize the ones we want to run on azure and then trigger if any python file in all of the test suite has changed?

Or just run our own script to see if any of those files were changed at the start of the main CI for an easy + fast exit with a pass.

new way to subset azure tests

c2b3203

handle azure specially

baee914

markcmiller86 and others added 10 commits November 21, 2024 09:29

create cksum file

c90ff40

action update prci cksum

96a0915

add "prci" tag

48b790d

use cksum trigger

d3835b0

adjust scripting

a673153

ensure branch is co

c585716

Update prci checksum

23255fe

Merge branch 'develop' into task-mcm86-19nov24-expand-azure-tests

337f833

fix syntax error

cbe3cc0

Merge branch 'task-mcm86-19nov24-expand-azure-tests' of github.com:vi…

5a3dcf0

…sit-dav/visit into task-mcm86-19nov24-expand-azure-tests

markcmiller86 requested review from JustinPrivitera, biagas and cyrush November 21, 2024 17:55

rm debug

0c0d2e7

JustinPrivitera previously approved these changes Nov 21, 2024

View reviewed changes

remove symlink stuff

83c500c

markcmiller86 dismissed JustinPrivitera’s stale review via 83c500c November 21, 2024 21:33

Update prci checksum

144393b

JustinPrivitera reviewed Nov 21, 2024

View reviewed changes

markcmiller86 closed this Nov 21, 2024

markcmiller86 deleted the task-mcm86-19nov24-expand-azure-tests branch November 21, 2024 21:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Testing new way to specify azure tests #20060

Testing new way to specify azure tests #20060

markcmiller86 commented Nov 20, 2024 •

edited

Loading

cyrush commented Nov 20, 2024 •

edited

Loading

markcmiller86 commented Nov 20, 2024

markcmiller86 commented Nov 21, 2024

JustinPrivitera Nov 21, 2024

markcmiller86 Nov 21, 2024

JustinPrivitera Nov 21, 2024

JustinPrivitera commented Nov 21, 2024

cyrush commented Nov 21, 2024

markcmiller86 commented Nov 21, 2024

JustinPrivitera commented Nov 21, 2024

markcmiller86 commented Nov 21, 2024

JustinPrivitera Nov 21, 2024

markcmiller86 commented Nov 21, 2024

cyrush commented Nov 22, 2024

Testing new way to specify azure tests #20060

Testing new way to specify azure tests #20060

Conversation

markcmiller86 commented Nov 20, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Reminders:

Checklist:

cyrush commented Nov 20, 2024 • edited Loading

markcmiller86 commented Nov 20, 2024

markcmiller86 commented Nov 21, 2024

JustinPrivitera Nov 21, 2024

Choose a reason for hiding this comment

markcmiller86 Nov 21, 2024

Choose a reason for hiding this comment

JustinPrivitera Nov 21, 2024

Choose a reason for hiding this comment

JustinPrivitera commented Nov 21, 2024

cyrush commented Nov 21, 2024

markcmiller86 commented Nov 21, 2024

JustinPrivitera commented Nov 21, 2024

markcmiller86 commented Nov 21, 2024

JustinPrivitera Nov 21, 2024

Choose a reason for hiding this comment

markcmiller86 commented Nov 21, 2024

cyrush commented Nov 22, 2024

markcmiller86 commented Nov 20, 2024 •

edited

Loading

cyrush commented Nov 20, 2024 •

edited

Loading