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

feat(fluxtest): enhance fluxtest to use package name with test/skip flags #5039

Merged
merged 8 commits into from
Aug 11, 2022

Conversation

skartikey
Copy link
Contributor

@skartikey skartikey commented Aug 2, 2022

The package name can be added to the testcase name as a qualifier
Format - package.name

Here is an example -
There are 2 testcase exist with name group

$find . -name '*.flux' | xargs grep 'testcase group '
./stdlib/experimental/group_test.flux:testcase group {
./stdlib/universe/group_test.flux:testcase group {

// testcase name without qualifier will run both the test.

go run ./cmd/flux test --test group

// testcase name with qualifier will run the specific test

go run ./cmd/flux test --test universe.group
go run ./cmd/flux test --test experimental.group

Adding a duplicate test name in the same package will produce an error -
testcase name "group", already exists in package "universe"

fixes: #4877

Checklist

Dear Author 👋, the following checks should be completed (or explicitly dismissed) before merging.

  • ✏️ Write a PR description, regardless of triviality, to include the value of this PR
  • 🔗 Reference related issues
  • 🏃 Test cases are included to exercise the new code
  • 🧪 If new packages are being introduced to stdlib, link to Working Group discussion notes and ensure it lands under experimental/
  • 📖 If language features are changing, ensure docs/Spec.md has been updated

Dear Reviewer(s) 👋, you are responsible (among others) for ensuring the completeness and quality of the above before approval.

@skartikey skartikey requested a review from a team as a code owner August 2, 2022 14:24
@skartikey skartikey requested review from wolffcm and removed request for a team August 2, 2022 14:24
Copy link

@wolffcm wolffcm left a comment

Choose a reason for hiding this comment

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

I had some comments but this looks good for the most part. I had a couple of comments that it would be good to address.

Let's make sure that downstream repos, which have their own skip lists, continue to work right.

cmd/flux/cmd/test.go Outdated Show resolved Hide resolved
cmd/flux/cmd/test.go Outdated Show resolved Hide resolved
cmd/flux/cmd/test.go Outdated Show resolved Hide resolved
@skartikey skartikey force-pushed the skartikey-flux-fluxtest-flag branch 2 times, most recently from 23a5cf8 to 9be3e2f Compare August 4, 2022 15:03
@skartikey skartikey requested a review from wolffcm August 4, 2022 15:05
Copy link

@wolffcm wolffcm left a comment

Choose a reason for hiding this comment

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

You are definitely close on this, but it would be good to have test case for the error that is produced when two cases have the same name in the same package. That will keep us from having any ambiguity moving forward, to it would be good to lock it down.

cmd/flux/cmd/test.go Show resolved Hide resolved
skartikey added a commit that referenced this pull request Aug 9, 2022
At the moment we print only the start and end location of the ast.
with this change filename will be included in the location print.

requred in #5039 to print
duplicate existing test with filename in the location.
skartikey added a commit that referenced this pull request Aug 10, 2022
…5057)

* fix: pass filename to the flux parser to get it printed in location

At the moment we print only the start and end location of the ast.
with this change filename will be included in the location print.

requred in #5039 to print
duplicate existing test with filename in the location.

* chore: make generate
@skartikey
Copy link
Contributor Author

skartikey commented Aug 10, 2022

After merging #5057 Did a manual test by duplicating the test case name in the same package and got the below error -

duplicate testcase name "group", found in package "universe", at locations stdlib/universe/group_test.flux|38:10-38:15 and stdlib/universe/group_ungroup_test.flux|48:10-48:15

@skartikey skartikey force-pushed the skartikey-flux-fluxtest-flag branch from 9be3e2f to abbbfe5 Compare August 10, 2022 18:40
@skartikey skartikey requested a review from wolffcm August 10, 2022 18:42

func Test_TestCmd_Error_Duplicate(t *testing.T) {
wantErr := errors.New("duplicate testcase name \"duplicate\", found in package \"test\", at locations testdataduplicate/test_test.flux|7:10-7:19 and testdataduplicate/test_test.flux|14:10-14:19")
runForPath(t, "./testdataduplicate", wantErr, "--test", "duplicate")
Copy link

Choose a reason for hiding this comment

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

Very nice!

@skartikey skartikey force-pushed the skartikey-flux-fluxtest-flag branch 3 times, most recently from 3577d8e to 21b863f Compare August 11, 2022 16:28
@skartikey skartikey force-pushed the skartikey-flux-fluxtest-flag branch from 21b863f to 1dc4589 Compare August 11, 2022 17:06
@skartikey skartikey merged commit def15ad into master Aug 11, 2022
@skartikey skartikey deleted the skartikey-flux-fluxtest-flag branch August 11, 2022 17:54
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.

fluxtest test case names may conflict
2 participants