ci: Only run functional tests on native windows in master

fjahr commented at 9:42 am on October 3, 2023: contributor

This idea was discussed here.

DrahtBot commented at 9:42 am on October 3, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto
Concept ACK	MarcoFalke, fanquake

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

DrahtBot added the label Tests on Oct 3, 2023

fjahr renamed this:
~~ci: Only run functional tests on windows in master~~
ci: Only run functional tests on native windows in master
on Oct 3, 2023

MarcoFalke commented at 9:51 am on October 3, 2023: member

Concept ACK.

Doesn’t seem ideal, but I can’t see a better alternative right now.

MarcoFalke requested review from hebasto on Oct 3, 2023

MarcoFalke added the label DrahtBot Guix build requested on Oct 3, 2023

MarcoFalke removed the label DrahtBot Guix build requested on Oct 3, 2023

hebasto approved

hebasto commented at 2:55 pm on October 3, 2023: member

ACK 9eca34224021d5adc80f09555b2e15a81ccd6c76, the diff is correct.

Suggesting to add a comment that explains the reasons of skipping functional tests in pull requests.

fjahr force-pushed on Oct 3, 2023

fjahr commented at 3:29 pm on October 3, 2023: contributor

I added a comment as suggested by @hebasto including TODO since that seemed most appropriate.

in .github/workflows/ci.yml:289 in ec2a67cea9 outdated

285@@ -286,6 +286,10 @@ jobs:
286         run: py -3 test\util\rpcauth-test.py
287 
288       - name: Run functional tests
289-        env:
290-          TEST_RUNNER_EXTRA: ${{ github.event_name != 'pull_request' && '--extended' || '' }}
291-        run: py -3 test\functional\test_runner.py --jobs $env:NUMBER_OF_PROCESSORS --ci --quiet --tmpdirprefix=$env:RUNNER_TEMP --combinedlogslen=99999999 --timeout-factor=$env:TEST_RUNNER_TIMEOUT_FACTOR $env:TEST_RUNNER_EXTRA
292+        # TODO: Only run functional tests in master.

MarcoFalke commented at 3:59 pm on October 3, 2023:

not sure about adding a TODO that is unclear on how to fix. GHA is a proprietary closed-source solution, so any debugging is hard and easily outdated.

Feel free to open a tracking issue, or use the existing thread, but I don’t think putting a TODO in the code is helpful. It is just spam that shows up when grep-ing for it.

fjahr commented at 4:46 pm on October 3, 2023:

I have removed the TODO. I can’t say I’m a fan of TODOs in the code in general but since we use them it would also be fine to use them as “this should still be investigated” or “maybe check again if this has been fixed by now” reminders. I will open a tracking issue once this is merged.

MarcoFalke commented at 4:53 pm on October 3, 2023:

The main problem with TODO’s is that once the status changes (one way or the other), they require changing the source code. This is fine for single-dev projects, however for collaborative projects it is better to simply provide the motivation/reason behind a change and then link to a thread/issue if needed, which can be easier tracked and updated with as much information as needed.

fjahr force-pushed on Oct 3, 2023

MarcoFalke approved

fanquake requested review from hebasto on Oct 4, 2023

in .github/workflows/ci.yml:289 in 62c7164bf5 outdated

285@@ -286,6 +286,10 @@ jobs:
286         run: py -3 test\util\rpcauth-test.py
287 
288       - name: Run functional tests
289-        env:
290-          TEST_RUNNER_EXTRA: ${{ github.event_name != 'pull_request' && '--extended' || '' }}
291-        run: py -3 test\functional\test_runner.py --jobs $env:NUMBER_OF_PROCESSORS --ci --quiet --tmpdirprefix=$env:RUNNER_TEMP --combinedlogslen=99999999 --timeout-factor=$env:TEST_RUNNER_TIMEOUT_FACTOR $env:TEST_RUNNER_EXTRA
292+        # Only run functional tests in master.

hebasto commented at 9:52 am on October 4, 2023:

I reckon this is not correct. The job will run on every push to our version branches and in personal repositories for any branch as well.

Maybe “Do not run functional tests for pull requests.” or similar?

fjahr commented at 2:22 pm on October 4, 2023:

Ah, ok, @MarcoFalke actually suggested to do what the comment says. So I instead adapted the if check to really only run in master (I hope). Does that work for you as well or should I rather change the comment?

hebasto commented at 2:25 pm on October 4, 2023:

So I instead adapted the if check to really only run in master (I hope).

Why skip version branches?

Also I found useful my personal repo CI in my everyday work.

UPD. The previous code was just fine as for me.

fjahr force-pushed on Oct 4, 2023

fanquake commented at 3:11 pm on October 4, 2023: member

Concept ACK. Lets get this merged, because this is just continuing to confuse and annoy contributors.

ci: Only run functional tests on windows in master aba4a5887b

fjahr force-pushed on Oct 4, 2023

fjahr commented at 3:48 pm on October 4, 2023: contributor

Reverted the if and fixed the comment.

hebasto approved

hebasto commented at 3:54 pm on October 4, 2023: member

ACK aba4a5887b44bf7cbee9ea0a8e02bb92c1b4147b

fanquake merged this on Oct 4, 2023

fanquake closed this on Oct 4, 2023

ci: Only run functional tests on native windows in master #28567

Reviews

Conflicts