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

Fix eng content tests; run tests via GitHub Actions #262

Merged
merged 13 commits into from
Jun 27, 2024
Merged

Fix eng content tests; run tests via GitHub Actions #262

merged 13 commits into from
Jun 27, 2024

Conversation

alexandear
Copy link
Contributor

@alexandear alexandear commented Jun 25, 2024

The PR fixes the test that checks code examples in the _content/tour/eng directory and adds GitHub Actions workflow to run tests on each PR or push to the main branch.

Now, running go test -v ./... works flawlessly.

I have divided the PR into separate logical commits to facilitate review. Each commit corresponds to one logical change.

Changes:

  • Fixed TestContent to work with //go:build build constraint. It's worth mentioning that the tour from Go team's uses // +build comments, but this gotour uses //go:build (introduced here drop legacy build tag #45). Thus, TestContent has been adapted to work with //go:build.
  • Moved content_test.go to the root because go test skips the _content folder when detecting tests.
  • Renamed TestContent to TestContentEngTour. Run tests only for eng directory.
  • Retained only one content_test.go that checks examples for all languages.
  • Added the missing OMIT to all code snippets.
  • Added the build constraint nobuild to code snippets that cannot be compiled via go build.
  • Added the build constraint norun to code snippets that cannot be executed. Note that we cannot use no-run (with a hyphen) as a build constraint name.

@alexandear
Copy link
Contributor Author

alexandear commented Jun 25, 2024

Follows #260


It's possible to check this PR locally via the GitHub CLI by executing the following command:

gh pr checkout 262 

@ardan-bkennedy
Copy link
Contributor

This needs to merge to bill/testing

@alexandear
Copy link
Contributor Author

It's possible to review this PR without merging it into bill/testing. For instructions, please visit Checking Out Pull Requests Locally.

@ardan-bkennedy
Copy link
Contributor

Not sure why? I will need to merge it into main anyway. I can easily do that in this testing branch.

@ardan-bkennedy
Copy link
Contributor

I can delete this branch and give you a new one to merge to?

@alexandear
Copy link
Contributor Author

Not sure why? I will need to merge it into main anyway. I can easily do that in this testing branch.

I'm assuming we are using the GitHub flow. This flow includes the following steps:

  1. Forking a repository: I forked ardanlabs/gotour into alexandear/gotour.
  2. Making changes and creating a PR: I made changes in alexandear:fix-content-test and created this PR.
  3. Reviewing the PR by maintainers and adding comments: You need to review the PR either using the GitHub UI or by checking it out locally.
  4. Addressing comments: I need to address your comments on my PR.
  5. Merging the PR into the default branch by maintainer: After I have addressed the comments, you need to merge this PR into main by pressing the button "Merge". GitHub will automatically mark the PR as "Merged".

@ardan-bkennedy
Copy link
Contributor

The PR is TOO BIG and it scares me. I have to review it and make sure nothing breaks. I need it in my testing branch I provided if you want to see this merged.

@alexandear alexandear changed the title Fix content tests; run tests via GitHub Actions Fix eng content tests; run tests via GitHub Actions Jun 26, 2024
@ardan-bkennedy
Copy link
Contributor

I created a new bill/testing2 branch for this PR.

@alexandear
Copy link
Contributor Author

I reverted all changes related to local translations (they will be fixed later) and retained only the fixes for examples in the eng directory to simplify the review process.

Only two files require thorough review:

  • .github/workflows/pr.yml
  • content_test.go

All other modifications add nobuild and norun build constraints to examples.

Also, the tests are passed on CI: https://github.com/ardanlabs/gotour/actions/runs/9681636341/job/26712671977?pr=262

@ardan-bkennedy
Copy link
Contributor

I will NOT being merging this into main directly. You will need to merge this into bill/testing2 so I can gain access to the code directly and run my own test and review. If you are not willing to do this, I can't accept it. I apprecaite the time and effort you are putting into this. That is why I am asking you for this again.

@alexandear alexandear changed the base branch from main to bill/testing2 June 27, 2024 11:53
@alexandear
Copy link
Contributor Author

@ardan-bkennedy changed branch from main to bill/testing2.

@ardan-bkennedy ardan-bkennedy merged commit bcbcc14 into ardanlabs:bill/testing2 Jun 27, 2024
1 check passed
@ardan-bkennedy
Copy link
Contributor

I made a some minor refactoring changes. This code is nice and I was able to see what you did. Pull down these changes and you can give me a PR to main and I will accept it. Make sure you have your name in the CONTRIBUTORS file please.

@alexandear alexandear deleted the fix-content-test branch June 27, 2024 16:05
@alexandear
Copy link
Contributor Author

Thank you. Created #263.

My name already in the CONTRIBUTORS file.

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