search

stateful / runme

DevOps Notebooks Built with Markdown
https://runme.dev
Apache License 2.0
1.11k stars 36 forks source link

Disable SONAR scan in PRs created from forks #591

Closed jlewi closed 4 months ago

jlewi commented 4 months ago

~~* 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

adambabik commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

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.

sourishkrout commented 4 months ago
  • 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 commented 4 months ago

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

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 commented 4 months ago

@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.

jlewi commented 4 months ago

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

sourishkrout commented 4 months ago

@jlewi are the pre commit hooks running for you (re https://github.com/stateful/runme/pull/591/commits/d4e0bdeb567feb097617978e19f3b37042b403ef)? Wondering if our contribution guidelines are incomplete

jlewi commented 4 months ago

@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.