Skip to content
This repository has been archived by the owner on Mar 27, 2024. It is now read-only.

fixed broken behavior of buffalo test -m #181

Merged
merged 3 commits into from
Jul 7, 2022
Merged

Conversation

sio4
Copy link
Member

@sio4 sio4 commented Jul 2, 2022

Not sure when the issue started, but buffalo test -m TEST was not working as expected. The bug was reported as #180 recently (transferred from the buffalo repository).

This fix fixes the following things:

  • removed unnecessary .buffalo.dev.yml existence checking on buffalo dev command. The logic should not reach there since the rootCmd checks that before executing subcommands.
  • fixed wrong anywhereCommands checking. anywhereCommands means they can be executed outside app root.
  • fixed not to register plugins as anywhereCommands. (maybe related to the above miss-checking?)
  • fixed buffalo test -m handles argument properly especially with testify.

fixes #180

@sio4 sio4 added the bug Something isn't working label Jul 2, 2022
@sio4 sio4 self-assigned this Jul 2, 2022
@sio4 sio4 requested a review from a team as a code owner July 2, 2022 16:05
@sio4 sio4 linked an issue Jul 2, 2022 that may be closed by this pull request
@sio4 sio4 enabled auto-merge (rebase) July 2, 2022 16:32
for _, freeCmd := range anywhereCommands {
if freeCmd != cmd.Name() {
if freeCmd == cmdName {
Copy link
Member Author

Choose a reason for hiding this comment

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

If the current command (subcommand) is one of anywhereCommands, just returns nil and stops prechecking. Otherwise, the command should be executed within the application directory that can be determined by checking .buffalo.dev.yml currently.

Copy link
Member Author

@sio4 sio4 left a comment

Choose a reason for hiding this comment

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

adding some comments to explain the changes.

cmdName := cmd.Name()
if cmd.Parent() != cmd.Root() {
// e.g. `completion` for `buffalo completion bash` instead of `bash`
cmdName = cmd.Parent().Name()
Copy link
Member Author

Choose a reason for hiding this comment

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

it is possible that some "current command" is not a direct child of root but a grandchild. they should be checked with the name of their parent.

@@ -28,7 +28,8 @@ func plugs() plugins.List {
func decorate(name string, cmd *cobra.Command) {
pugs := plugs()
for _, c := range pugs[name] {
anywhereCommands = append(anywhereCommands, c.Name)
Copy link
Member Author

Choose a reason for hiding this comment

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

plugins are not anywhere commands. they should be executed within the app directory.

@@ -94,20 +92,9 @@ func runDevScript(ctx context.Context, app meta.App) error {
func startDevServer(ctx context.Context, args []string) error {
app := meta.New(".")

cfgFile := "./.buffalo.dev.yml"
Copy link
Member Author

Choose a reason for hiding this comment

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

dev command is not an anywhere command, so if the command was executed from the directory that has no .buffalo.dev.yml, the execution will be aborted by the root's pre-checking and this block should not be reachable.

@@ -156,11 +156,12 @@ func (m mFlagRunner) Run() error {
continue
}

cmd := newTestCmd(m.args)
Copy link
Member Author

Choose a reason for hiding this comment

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

a command should be built within the app's root so the correct configuration was applied.

@@ -178,8 +179,9 @@ func (m mFlagRunner) Run() error {
return nil
}

func hasTestify(p string) bool {
cmd := exec.Command("go", "test", "-thisflagdoesntexist")
func hasTestify(args []string) bool {
Copy link
Member Author

Choose a reason for hiding this comment

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

the base of the checking command also should be a complete command (except the last wrong argument which causes an intentional error) so the execution can be guaranteed.

@sio4 sio4 requested a review from paganotoni July 6, 2022 09:56
@paganotoni
Copy link
Member

This -m flag is something I've been thinking about while working on the new CLI architecture. I've been thinking if this is something standard in Go or something that Testify requires and how do we write our CLI so we are not dependent on Testify.

@sio4
Copy link
Member Author

sio4 commented Jul 6, 2022

This -m flag is something I've been thinking about while working on the new CLI architecture. I've been thinking if this is something standard in Go or something that Testify requires and how do we write our CLI so we are not dependent on Testify.

In our code, the -m works as two ways.

  • if the code can detect testify-enabled on a module, the -testify.m TestCase flag is used instead -m itself.
  • if the module to be tested is not testify-enabled, the -run TestCase flag is used instead of -m itself.

So what I found for -m is a kind of meta flag to be transformed into the standard -run or testify style and I think this method is useful. (Actually, the scaffolded app supports testify)

@paganotoni
Copy link
Member

Was taking the time to review how this impacted the v1 version of all this. Thanks for adding the comments @sio4.

@sio4 sio4 merged commit 4bb0904 into development Jul 7, 2022
@paganotoni paganotoni deleted the fixing-buffalo-test-m branch July 7, 2022 12:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

buffalo test -m Test commands fail when using sqlite3 databases
2 participants