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

build: run example tests during CI workflow #1030

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

crazybolillo
Copy link

A new test has been added in order to verify that all code under
_examples is up to date and has not broken due to API changes.

This test runs as the last step of the test.yml CI workflow.

Closes #912.

@pjbgf
Copy link
Member

pjbgf commented Feb 21, 2024

@crazybolillo thanks for working on this. But I believe this were made redundant by the changes recently merged from #446. More specifically cc10b2a.

@crazybolillo
Copy link
Author

That is a nice PR. I have two comments/questions

  1. Seems like examples tests are still not being run on the CI? From what I understand, the test-coverage target does not run them.
  2. Maybe ignored tests can be built but not run? I don't know if this is worth it, but it could catch some issues like the ls example using a deprecated version of commitgraph.

Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@crazybolillo you are right, the examples were not being executed at CI. I proposed a few changes to this PR that aims to reuse the existing code to achieve the same results, PTAL.

Comment on lines 39 to 41
env:
EXAMPLES_DIR: _examples
run: go test -v -run='^TestBuildExamples$'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
env:
EXAMPLES_DIR: _examples
run: go test -v -run='^TestBuildExamples$'
run: make test

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since "normal" tests are already being run as part of the coverage, isn't it better to just run the example tests for this new step? It would make the CI faster.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The suggestion for make test was because that would also introduce -race, which we are not doing at the moment.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I have changed it to make test, however a test is failing. I will look into it to see why.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @crazybolillo, let's keep the previous change you had go test -v -run='^TestBuildExamples$'.

The make test has some intermittent issues due to the -race. Feel free to propose a separate PR for that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late reply. I have gone back to just the build examples. Will see if I can create a separate PR to fix the flakiness from -race

.github/workflows/test.yml Outdated Show resolved Hide resolved
build_examples_test.go Outdated Show resolved Hide resolved
@crazybolillo crazybolillo requested a review from pjbgf March 11, 2024 02:25
@crazybolillo crazybolillo force-pushed the i912-crazybolillo branch 3 times, most recently from 059da7c to 383b97b Compare March 11, 2024 13:43
@crazybolillo crazybolillo changed the title tests: verify code in _examples build: run make test during CI workflow Mar 11, 2024
Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Thanks @crazybolillo! 🥇

@crazybolillo crazybolillo force-pushed the i912-crazybolillo branch 2 times, most recently from 37db86c to cfc0de2 Compare March 15, 2024 23:05
@crazybolillo crazybolillo changed the title build: run make test during CI workflow build: run example tests during CI workflow Mar 15, 2024
@crazybolillo crazybolillo requested a review from pjbgf March 15, 2024 23:06
Tests for examples exist, however they were not being run as part of the CI.
This commit fixes it by adding a new step in the test workflow which runs said
tests.

Related to go-git#912.
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.

Create CI tests to confirm all examples build
2 participants