Task improving test #247

paganotoni · 2022-11-25T16:49:15Z

What is being done in this PR?

This PR rewrites our test command and adds tests for the cases that it used to cover. It also adds some helpful instructions needed to understand some of the flags that have been there forever. While on it I changed the way we determine the packages that need to be tested.

This is related to #241.

What are the main choices made to get to this solution?

Decided to simplify it and stick as close as possible to undocumented behaviors in the past implementation.

What was discovered while working on it? (Optional)

Learned how the Buffalo test command uses the -testify.m flag and found it a bit conflicting with the -run flag.

List the manual test cases you've covered before sending this PR:

I ran this command against a sample application and validated that it was causing the correct commands being run.

paganotoni · 2022-12-01T18:42:37Z

@sio4 I would love to get your blessing before merging this one. Whenever you have the chance please take a look at it.

sio4 · 2022-12-02T14:38:52Z

@sio4 I would love to get your blessing before merging this one. Whenever you have the chance please take a look at it.

Sorry for the late response. I was not able to take a look at this PR during the weekdays, but I will check it soon!

sio4

I took a look at the PR and found some issues with it. I feel like this approach could be a good one with backward compatibility if we fixed some listed issues. However, I would like to discuss a more basic concept regarding command line options (which was one of my rough ideas for v2 when I worked on #181 and #167). I will leave a comment including that on #241.

internal/cmd/test/build_cmd.go

sio4 · 2022-12-03T01:48:57Z

internal/cmd/test/build_cmd.go

+	// testValuedFlags are the flags on the go test command that
+	// receive a value, we use this to determine if the last argument
+	// is a flag or a package path
+	testValuedFlags = strings.Join([]string{


I am not sure if having a static list of flags is a good way to maintain especially since we support multiple go versions which could have different sets of flags. For example, -covermode and -coverpkg are missing.

Also, this approach does not support -args flag even though I am not sure how many developers use this flag with the apps built with Buffalo.

Agreed. I don't like this part. I added this to be able to understand when a flag received values, this is important to determine if the test command is being passed packages to test. p.e:

go test // go test -run xxx // No packages go test -run xxx -p 1 // No packages go test ./... // packages specified

Yes, I know this is added to check if the user gave the module list to test or just gave flags (with values).

My idea on this is simple. I think we can just ignore adding an automatically found module list when the user gave additional flags for go test. Previously when we need to walk through the given (or automatically collected) modules for -m supporting before, with switching between -testify.m and -run, so the list of target modules was important. However, if we deprecate the behavior of -m we had and just use the standard -run, we don't need to care about it anymore and we can let the user complete the command line as the same as when the user runs go test directly.

Briefly, I would like to make the command the same or similar to the go test command line:

buffalo test -- -cover ./... == go test -cover ./... after preparing test db (just use the user-provided args as is)

buffalo test == go test after preparing test db (just use the user-provided args as is, which is none)

or buffalo test == go test ./... as a special exception (add ./... for when there is no args at all)

So make buffalo test [...] equivalent to go test [...] except for the database preparing part, it could be easy to understand the behavior since all Buffalo users are Go developers, and it is more expectable.

sio4 · 2022-12-03T02:02:50Z

internal/cmd/test/build_cmd.go

+	}
+
+	// Add extra args (-testify.m) to the command
+	cargs = append(cargs, extraArgs(args)...)


I found that the function clean() (only) removes -m and -testify.m from the args and the function extraArgs() appends -testify.m at the end of the command line after the list of packages when one of -m, -testify.m, or -run was specified. However, it will not work if the user provides -run TestMethod since when a user wants to run a specific test case in a single Suite, the proper command line should be something like go test -run TestSuite -testify.m TestMethod. (see another comment for the old code)

Also, -run does not always mean to be -testify.m. E.g. I have a package named utils in my app which does not use testify (suite) based test cases which to be run with the standard -run TestUtilsFunc.

Yes. The biggest issue I'm facing is maintaining a functionality that I don't see documented by our tests.

Right, we just mentioned the feature in short words, we just supported a simple use case only, and that is for the season of v0.10, which is the season around Go v1.7 or v1.8 maybe. I think we could adapt the testing to the modern version of Go and Buffalo.

https://gobuffalo.io/documentation/guides/testing/#execute-a-single-test

Yes. One consideration to bring here is that the -m feature can be addressed with -run, developers just have to enter a larger name for the tests. In general I would love to see Buffalo being closer to the stdlib for our upcomming versions.

Agreed. I would like to make it easy for Go developers who already know how go works.

sio4 · 2022-12-03T02:08:45Z

internal/cmd/test/test.go

-		p = strings.TrimPrefix(p, app.PackagePkg+string(filepath.Separator))
-		os.Chdir(p)
-
-		if hasTestify(cmd.Args) {


We previously used this "for each package loop" approach as a workaround for the -testify.m conditions:

just with the -testify.m TestMethod only without -run TestSuite,

per package switching of the flag between -run and -testify.m with the (quick and dirty) hasTestify() helper.

Right. IMO we should move to run tests closer to the standard library. At this point, just to support this -testify.m behavior we're disallowing users from passing arguments to the go test command.

sio4 · 2022-12-03T02:40:53Z

internal/cmd/test/test.go

+
+		// DisableFlagParsing is set to true since we will need to allow undefined
+		// flags to be passed to the go test command.
+		DisableFlagParsing: true,


Actually, I would like to change this behavior.

Cobra does not support changing this behavior. AFAIK if we enable this we will have to listen for each of the flags that the go build and test command accept, otherwise Cobra will err.

I explained more clearly (with my limited Yonglish skill) in the issue #241. In short, We can use "end-of-options" for this. Cobra supports the POSIX standard "end-of-options" flag which is the double dash --, and using it could be acceptable for most Unix-like OS users naturally. By using this, we can both use Cobra's flag parsing for Buffalo-specific flags and pass others to go test very easily.

That's interesting. I did not try that but when I added test flags that were not declared in our Cobra command got error. Will take a look at end-of-options.

Yeah, the end-of-options is the POSIX standard and it will make us and users easy to separate buffalo-specific flags such as --force-migrations and go test flags including app-specific flags with the standard flag -args. However, I am still open to a simpler way too:

POSIX style with simpler internal with Cobra (Cobra makes the args for the go test)

$ buffalo test --force-migration -- -cover -race ./...

vs.

Go style without end-of-options but with slightly more work of cleaning buffalo-specific args from the original args

$ buffalo test --force-migration -cover -race ./...

sio4 · 2022-12-03T07:36:56Z

Hi @paganotoni,

I had time to check this PR and also some background histories and tests for the issue and I left some comments on the issue #241. Please check those comments, I tried to describe in detail what I think about the issue and the underlying consideration for the command line style.

github-actions · 2023-01-24T02:26:01Z

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment. Otherwise, this will be closed in 7 days.

github-actions · 2023-01-31T02:33:27Z

This PR was closed because it has been stalled for 45+7 days with no activity.

paganotoni added 7 commits November 22, 2022 15:21

task: improving test command

6f7275c

task: rewriting our test command

3e7f5d4

feature: adding -testify.m flag

36cbb2c

task: should be -run

c627414

task: adding a few tests

41198df

cleaning

f835bf6

task: adding some help on the test command

6782c95

paganotoni requested a review from a team as a code owner November 25, 2022 16:49

paganotoni enabled auto-merge November 25, 2022 16:49

paganotoni requested a review from sio4 November 26, 2022 14:53

sio4 reviewed Dec 3, 2022

View reviewed changes

sio4 mentioned this pull request Dec 9, 2022

feature: pass test flags through to go test #241

Open

github-actions bot added the stale label Jan 24, 2023

github-actions bot added the s: closed label Jan 31, 2023

github-actions bot closed this Jan 31, 2023

auto-merge was automatically disabled January 31, 2023 02:33
Pull request was closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Task improving test #247

Task improving test #247

paganotoni commented Nov 25, 2022

paganotoni commented Dec 1, 2022

sio4 commented Dec 2, 2022

sio4 left a comment •

edited

Loading

sio4 Dec 3, 2022

paganotoni Dec 7, 2022

sio4 Dec 9, 2022

sio4 Dec 3, 2022

paganotoni Dec 3, 2022

sio4 Dec 4, 2022

paganotoni Dec 7, 2022 •

edited

Loading

sio4 Dec 9, 2022

sio4 Dec 3, 2022

paganotoni Dec 3, 2022

sio4 Dec 3, 2022

paganotoni Dec 3, 2022

sio4 Dec 4, 2022

paganotoni Dec 7, 2022

sio4 Dec 9, 2022

sio4 commented Dec 3, 2022

github-actions bot commented Jan 24, 2023

github-actions bot commented Jan 31, 2023

Task improving test #247

Task improving test #247

Conversation

paganotoni commented Nov 25, 2022

What is being done in this PR?

What are the main choices made to get to this solution?

What was discovered while working on it? (Optional)

List the manual test cases you've covered before sending this PR:

paganotoni commented Dec 1, 2022

sio4 commented Dec 2, 2022

sio4 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paganotoni Dec 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sio4 commented Dec 3, 2022

github-actions bot commented Jan 24, 2023

github-actions bot commented Jan 31, 2023

sio4 left a comment •

edited

Loading

paganotoni Dec 7, 2022 •

edited

Loading