Navigation Menu

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

Jump to bottom

Add --filter flag #138

Merged
merged 8 commits into from Jul 17, 2020
Merged

Add --filter flag #138

merged 8 commits into from Jul 17, 2020

Conversation

SimonBaeumer
Copy link
Member

@SimonBaeumer SimonBaeumer commented Jul 5, 2020 •
edited

Fixes #137 #77

Checklist

  • Added unit / integration tests for windows, macOS and Linux?
  • Added a changelog entry in CHANGELOG.md?
  • Updated the documentation (README.md, docs)?
  • Does your change work on Linux, Windows and macOS?
  • Add regex support
  • (?) Add wildcard support
  • Fix directory support
    • Maybe print warning that no test was found in file

@codeclimate
Copy link

codeclimate bot commented Jul 6, 2020

Code Climate has analyzed commit e73d4dd and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 89.4% (50% is the threshold).

This pull request will bring the total coverage in the repository to 91.7% (0.0% change).

View more on Code Climate.

@SimonBaeumer
Copy link
Member Author

@dylanhitt would you like to do a review?

@dylanhitt
Copy link
Member

Sure, I'll get to it by the end of Tuesday.

dylanhitt
dylanhitt reviewed Jul 8, 2020
View reviewed changes
Copy link
Member

@dylanhitt dylanhitt left a comment

Choose a reason for hiding this comment

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

Some requests, and I'm sure you planned on getting to it.

  1. Some integration tests
  2. So for now we're we'd be panicing if no filter was found in a directory. I like the idea of info/warning there. However, the goroutine makes this rough to deal with :( I think the Move test execution to runner file, refactor output #126 would make handling this warning much easier as well... yet again.
  3. I see you marked you added docs. Do you plan on updating the README?

I like the feature. I'm neither here nor there about wildcard support. I like this much. more than just passing a string at the end of test command 👍

pkg/app/app.go Outdated Show resolved Hide resolved
pkg/runtime/runtime.go
@@ -29,6 +29,8 @@ const (
Skipped
)

type Filters []string
Copy link
Member

Choose a reason for hiding this comment

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

Small point. Would this be better in suite.go?

Copy link
Member

Choose a reason for hiding this comment

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

This is really my only point. Is there a reason why you chose to place this here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I placed it in runtime because the filtering logic should be inside the runtime in the future.
The suite would be the wrong place because the filters are not defined in the suite, instead filters are only provided viá the command line.

dylanhitt
dylanhitt reviewed Jul 8, 2020
View reviewed changes
pkg/app/app.go Outdated Show resolved Hide resolved
@SimonBaeumer
Copy link
Member Author

Some requests, and I'm sure you planned on getting to it.

  1. Some integration tests
  2. So for now we're we'd be panicing if no filter was found in a directory. I like the idea of info/warning there. However, the goroutine makes this rough to deal with :( I think the Move test execution to runner file, refactor output #126 would make handling this warning much easier as well... yet again.
  3. I see you marked you added docs. Do you plan on updating the README?

I like the feature. I'm neither here nor there about wildcard support. I like this much. more than just passing a string at the end of test command

  1. Yes, should be added :)
  2. Thought about it too, I will add it, wasn't quite sure about it
  3. Did not updated the README because it is documented in the test command directly in the source code. Would you like to see this documented in the README? Maybe I would add the usage of the test command to the related section

@SimonBaeumer
Copy link
Member Author

I would like to freeze this pull request until your #126 and #140 are merged because they are very fundamental and could break a lot of things.

@SimonBaeumer
Copy link
Member Author

Ready for review :)
Did not implemented the warning if a file did not contain a test if it is filtered:

  • A bigger refactoring would be necessary because the filename is required in the execute function
  • If it is filtered I want a less verbose output. If verbose is enabled it could be logged, but atm it is too much effort

dylanhitt
dylanhitt previously requested changes Jul 16, 2020
View reviewed changes
pkg/runtime/runtime.go
@@ -29,6 +29,8 @@ const (
Skipped
)

type Filters []string
Copy link
Member

Choose a reason for hiding this comment

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

This is really my only point. Is there a reason why you chose to place this here?

pkg/app/app.go Outdated Show resolved Hide resolved
cmd/commander/commander.go Outdated Show resolved Hide resolved
@dylanhitt dylanhitt dismissed their stale review July 16, 2020 01:19

Did not mean to request changes

dylanhitt
dylanhitt reviewed Jul 16, 2020
View reviewed changes
Copy link
Member

@dylanhitt dylanhitt left a comment

Choose a reason for hiding this comment

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

I've left a few comments on some ideas. If you would like to address them you can. I mainly want your thoughts on this one and this one

SimonBaeumer
SimonBaeumer commented Jul 16, 2020
View reviewed changes
Copy link
Member Author

@SimonBaeumer SimonBaeumer left a comment

Choose a reason for hiding this comment

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

  • Using StringSliceFlag for filter flag now.
  • Need more explanation on the TextContext, could not follow along with it

cmd/commander/commander.go Outdated Show resolved Hide resolved
pkg/app/app.go Outdated Show resolved Hide resolved
@SimonBaeumer
Copy link
Member Author

I am confused with github code reviews, sometimes I can comment on requests, sometimes not.
My answers are displayed alone without context, it is really annoying :(
Or I do not understand it

@dylanhitt
Copy link
Member

I agree for some reason I can only add comments to my original comments. Also I cleared up what I meant about TextContext I was trying to say TestCommandContext. After your thoughts on that I think we'd be good to go.

@codeclimate
Copy link

codeclimate bot commented Jul 17, 2020

Code Climate has analyzed commit cb91de1 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 88.8% (50% is the threshold).

This pull request will bring the total coverage in the repository to 92.2% (-0.2% change).

View more on Code Climate.

@SimonBaeumer SimonBaeumer merged commit d896521 into master Jul 17, 2020
@SimonBaeumer SimonBaeumer deleted the add-filter-flag branch July 17, 2020 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dylanhitt dylanhitt dylanhitt left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

"Test may not be filtered when --dir is enabled"
2 participants
@SimonBaeumer @dylanhitt