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

Extend selftest workflow to multiple runners #9

Merged
merged 9 commits into from
Nov 4, 2022

Conversation

norwd
Copy link
Contributor

@norwd norwd commented Oct 27, 2022

Please describe the change you are making

I'm condensing the existing self-test and self-test-macos jobs into a single job that defines runs-on with a matrix. I'm also adding windows to the OS list so that now there is also a test case for windows runners. This should help catch errors like #4 and #8 by ensuring that gotestfmt can run on all the major OS runners for GitHub Actions.

Your code will be released under the MIT license. Are you in the position, and are you willing to release your code under this license?

Yup :)

Signed-off-by: Yuri Norwood 106889957+norwd@users.noreply.github.com

norwd added 2 commits October 28, 2022 11:23
This should help catch errors like GoTestTools#4 and GoTestTools#8 by ensuring that gotestfmt can run on all the major OS runners for GitHub Actions.

Signed-off-by: Yuri Norwood <106889957+norwd@users.noreply.github.com>
Extend selftest workflow to multiple runners
@norwd
Copy link
Contributor Author

norwd commented Oct 27, 2022

You can view an example run of the new workflow on my fork: norwd#1

Note that the MacOS job fails due to rate limiting because it's running in a fork and Windows fails due to #8. Once merged the MacOS job should run fine just fine, and once #8 is fixed ditto for Windows :)

@norwd
Copy link
Contributor Author

norwd commented Oct 29, 2022

Hmm, further experimentation shows that the runs: | script uses bash only feature that fail on the Windows runner. I'll update the PR with a pwsh version in the morning :)

norwd added 5 commits October 31, 2022 08:43
This allows `bash` and `pwsh` syntax to be used separately to avoid having to write the test as a polyglot 😬

Also added `secrets.GITHUB_TOKEN` as a fallback in case the repo secret `secrets.GH_TOKEN` is not set up, e.g. in a fork.

Signed-off-by: Yuri Norwood <106889957+norwd@users.noreply.github.com>
On MacOS, this results in:

```
Error: Provided rootDirectory /tmp is not a valid directory
```

On Windows this results in:

```
Error: No files were found with the provided path: /tmp/gotest.windows.log. No artifacts will be uploaded.
```

Both seem to be unhappy with `/tmp`, since the workspace is implicitly cleaned up at the end of the workflow, there shouldn't be a need to specify `/tmp`.

Signed-off-by: Yuri Norwood <106889957+norwd@users.noreply.github.com>
Signed-off-by: Yuri Norwood <106889957+norwd@users.noreply.github.com>
Signed-off-by: Yuri Norwood <106889957+norwd@users.noreply.github.com>
Separate windows step from *nix runners
@norwd
Copy link
Contributor Author

norwd commented Oct 30, 2022

Ok, I've refactored the workflow a bit so that the bash and pwsh scripts are separated (waayyyyy easier than trying to write a script that is simultaneously both vaid bash and valid powershell 😂). Now the only issue is that the Windows runner is still failing due to the error I reported in #8.

image

If you'd like to merge my changes now I think the new workflow still could be useful, the only thing that would be needed is changing the branch protection rules to require Self-test / on ubuntu and Self-test / on macos instead of on Ubuntu and on MacOS. Later on Self-test / on windows could also be added.

Signed-off-by: Yuri Norwood <106889957+norwd@users.noreply.github.com>
@norwd
Copy link
Contributor Author

norwd commented Nov 1, 2022

I've updated the workflow to use bash explicitly, as it is the default on Ubuntu and Macos, but on Windows pwsh is the default. I recently learned that the Windows runner also supports bash officially, which means my little workaround is not necessary 😄. Instead, all three runners now use the same bash script step.

.github/workflows/selftest.yml Outdated Show resolved Hide resolved
.github/workflows/selftest.yml Outdated Show resolved Hide resolved
@engelmi
Copy link
Member

engelmi commented Nov 2, 2022

Hi @norwd, thanks for your contribution! I really like the matrix to slim down the necessary code. I left two minor comments and as soon as they are resolved we can merge it, I'd say 😄
The gotestfmt-action does not work on windows at the moment - just as you reported in #8. I'll provide you with the details for the necessary changes in the issue if you are up for it 😉

Signed-off-by: Yuri Norwood <106889957+norwd@users.noreply.github.com>
@norwd norwd requested a review from engelmi November 2, 2022 20:52
@norwd
Copy link
Contributor Author

norwd commented Nov 2, 2022

Thanks @engelmi, I've applied your suggested changes. I'd be happy to look into solving #8 if you have any ideas?

@engelmi engelmi merged commit 19aa3d4 into GoTestTools:main Nov 4, 2022
engelmi pushed a commit that referenced this pull request Dec 2, 2022
* Extend selftest workflow to multiple runners

This should help catch errors like #4 and #8 by ensuring that gotestfmt can run on all the major OS runners for GitHub Actions.

Signed-off-by: Yuri Norwood <106889957+norwd@users.noreply.github.com>

* Separate windows step from *nix runners

This allows `bash` and `pwsh` syntax to be used separately to avoid having to write the test as a polyglot 😬

Also added `secrets.GITHUB_TOKEN` as a fallback in case the repo secret `secrets.GH_TOKEN` is not set up, e.g. in a fork.

Signed-off-by: Yuri Norwood <106889957+norwd@users.noreply.github.com>

* Remove `/tmp` directory for artifacts

On MacOS, this results in:

```
Error: Provided rootDirectory /tmp is not a valid directory
```

On Windows this results in:

```
Error: No files were found with the provided path: /tmp/gotest.windows.log. No artifacts will be uploaded.
```

Both seem to be unhappy with `/tmp`, since the workspace is implicitly cleaned up at the end of the workflow, there shouldn't be a need to specify `/tmp`.

Signed-off-by: Yuri Norwood <106889957+norwd@users.noreply.github.com>

* Name each workflow run of the matrix

Signed-off-by: Yuri Norwood <106889957+norwd@users.noreply.github.com>

* Fix artifact path

Signed-off-by: Yuri Norwood <106889957+norwd@users.noreply.github.com>

* Simplify workflow by just using bash universally

Signed-off-by: Yuri Norwood <106889957+norwd@users.noreply.github.com>

* Apply suggestions from code review

Signed-off-by: Yuri Norwood <106889957+norwd@users.noreply.github.com>

Signed-off-by: Yuri Norwood <106889957+norwd@users.noreply.github.com>
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.

2 participants