Disable SONAR scan in PRs created from forks

[jlewi]

• Disable the SONAR scan in PRs created from forks. It won't be able to run because the GHA can't get the secrets for PRs created from a fork.
  • The failure creates noise in various dashboards because we see the red "x" by the PR.

~~* A lot of unittests aren't running on CI because the GHA currently ~~

~~ * Runs all tests but only on windows and a bunch of tests have the build tag !windows~~
~~ * On docker ubuntu only the tests with the dag docker ubuntu run~~

* The fix is to add a GHA step that runs make test to run on all OSS
~~ * This rule doesn't use any build tags so it will run all tests.~~
Related to #587

• Mark some locally flaky tests to be skipped

[adambabik]

The first bullet points are not true. There is this step:

      - name: Test
        run: |
          export SHELL=/bin/bash
          export TZ=UTC
          TAGS="test_with_docker" make test/coverage
          make test/coverage/func

It runs all tests thanks to TAGS="test_with_docker" make test/coverage, including generating a coverage report.

Not all tests can run on Windows because they are Linux specific. At the current state, runme does not provide full Windows support. Windows with WSL is recommended.

With regard to the Sonar support and PRs from forks, let's wait for @sourishkrout's comment.

[jlewi]

@adambabik Thank you. You're correct and setting TAGS=tags_with_docker should run all tests. In which case, do you know what's going on with respect to service_test.go is running CI?
You can see from my previous run of test all
https://github.com/stateful/runme/actions/runs/9231990310/job/25402625003

That service_test failed in the test_all rule but not on main.
Based on the coverage report (https://github.com/stateful/runme/actions/runs/9229617830/job/25396164592) it does seem like service_test is running on maining.

It looks like test all sets -race=false whereas the test with docker rule doesn't.

[sourishkrout]

With regard to the Sonar support and PRs from forks, let's wait for @sourishkrout's comment.

Makes sense to disable Sonar for forks since we don't want to get into secrets sharing.

[adambabik]

In which case, do you know what's going on with respect to service_test.go is running CI?

It is run on Linux but it's omitted on Windows.

You can see from my previous run of test all https://github.com/stateful/runme/actions/runs/9231990310/job/25402625003

This test is simply flaky. It is run in main and on PRs but it sometimes fails due to race condition. runnerv2 is more stable in this respect.

It looks like test all sets -race=false whereas the test with docker rule doesn't.

Where do you see that? All tests are run with -race=false or it's omitted. When it's omitted it defaults to false.

[adambabik]

This is duplication of what's below (name: Test) but without coverage. It runs exactly the same set of tests.

We can argue if it should be TAGS="test_with_docker" make test-coverage (notice instead of - which is a different Makefile target) in the step below to bring it closer to make test.

[adambabik]

I don't understand this comment. It is set before calling make test: Set-Timezone -Id "UTC" -PassThru.

[sourishkrout]

I believe it's just a note to make clear that Set-Timezone is a Windows PowerShell command; for folks who are not familiar with Windows details. Please confirm @jlewi.

[jlewi]

That's right. Happy to remove it if you prefer less comments.

[adambabik]

My recommendation to move forward with this PR is as follows:

• Change TAGS="test_with_docker" make test/coverage to TAGS="test_with_docker" make test-coverage to make it closer to make test on Windows.
• Keep the change related to skipping SonarCloud scan.
• Remove other changes.

[sourishkrout]

• Remove other changes.

Please see my other comment. Btw, @adambabik, Jeremy discussed these changes with me. We're open to suggestions, however, let's move forward with what my eyes is a pragmatic solution.

[jlewi]

Thanks @adambabik and @sourishkrout and sorry for the slow response.
So I'm unsure what to do with this PR because. The original motivation for this PR was

• I observed locally that several tests were flaky
• On CI the tests were passing consistently on main.

This lead me to conclude that the problem was that some tests were not running under CI. This conclusion was based on a misunderstanding about how tags worked. I assumed that setting tag "test_with_docker" only ran those tests as opposed to all tests + tests with that tag.

As @adambabik points out that's incorrect. So TestRunnerServiceServerExecute_WithInput/SimulateCtrlC is running and presumably passing fairly reliably on CI. I tried again locally on main and its failing for me locally.

    --- FAIL: TestRunnerServiceServerExecute_WithInput/SimulateCtrlC (1.52s)
        service_execute_test.go:724: 
            	Error Trace:	/Users/jlewi/git_runme/internal/runnerv2service/service_execute_test.go:724
            	Error:      	"rpc error: code = Unknown desc = exit status 1" does not contain "exit status 130"
            	Test:       	TestRunnerServiceServerExecute_WithInput/SimulateCtrlC
        service_execute_test.go:725: 
            	Error Trace:	/Users/jlewi/git_runme/internal/runnerv2service/service_execute_test.go:725
            	Error:      	Not equal: 
            	            	expected: 130
            	            	actual  : 1
            	Test:       	TestRunnerServiceServerExecute_WithInput/SimulateCtrlC

So at this point; I don't know why CI is more reliable than local. As a result, I don't know how to fix GHA to make it not mask flakiness. However, this means all my CI changes (except for the sonar one) are solving a non-existent problem so I reverted them.

I'm uncertain what to do about skipping SimulateCtrlC. So I'm good with whatever you want. Skipping the test made more sense if we could reliably reproduce the failure in CI and didn't want to block PRs while we fixed it. But since the failures aren't reliably reproducing in CI the rationale doesn't really hold.

I've left the Sonar change in because I think that makes sense given reasons mentioned previously.

Closing this PR is also a completely reasonable outcome.

[adambabik]

@jlewi thanks for the detailed explanation! There could be several reasons why this test fails for you locally, such as bash version, OS (including different Linux distros), terminal settings, and so on.

One of the options we could introduce is to run tests locally in a Docker container. Then we could ensure more reliable behaviour for all contributors, without them having to guess what to install in which version or delaying feedback and waiting for the CI result. However, the fact is that the nature of runme and its ability to execute commands on the host OS will keep it open for edge cases.

[adambabik]

One suggestion, otherwise LGTM. Thanks!

[jlewi]

Thanks @adambabik ; I had no idea that function was part of the stdlib. I've applied the suggestion.

[sourishkrout]

👍 LGTM

[sourishkrout]

@jlewi are the pre commit hooks running for you (re d4e0bde)? Wondering if our contribution guidelines are incomplete

[jlewi]

@sourishkrout That was my mistake. I just accepted the suggestion through the GitHub UI and didn't check back to make sure the CI was passing. I'd have to double check that precommits are running on stateful/runme; they run on stateful/runme-vscode.