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

Move test execution to runner file, refactor output #126

Merged
merged 14 commits into from
Jul 12, 2020
Merged

Move test execution to runner file, refactor output #126

merged 14 commits into from
Jul 12, 2020

Conversation

dylanhitt
Copy link
Member

@dylanhitt dylanhitt commented Jun 14, 2020

Closes #121.

TODO:

  • Migrate runtime.Start to it's own file runner.go
  • Stream TestResults to output in runtime
  • Aggregate TestResults to a single struct Result
  • Add FileName to TestCase struct when executing against a dir
  • Define a single OutputWriter to be used in both Runtime and for out.PrintSummary
  • Test runner.go
  • Converge struct Results in Test_Command when executing against a dir
  • Switch output to go/template

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?

@dylanhitt
Copy link
Member Author

dylanhitt commented Jun 16, 2020

Mainly just trying to organize my thoughts.

I elected to implement runner.go in the runtime package do to moving runner into its own package would require a refactor of runtime. Pulling the execution out currently causes a circular dependency as all the executors reside in runtime. runtime being broken up would beneficial as well, but is out of the scope of this PR and the issue associated

We may benefit creating a Test_Command struct. We could store command ctx, OutputWriter, and test filters. We could provide a validation function make sure the command is valid. We could also use output to print logs from the Test_Command as well. Currently I just add OutputWriter as a global var.

--dir has a special case where we need to pass FileName to TestCase or TestResult. There's really no place to do this on TestResult as execution has been moved to runner.go. I elected to implement suite.ParseYAML(content, fileName) giving us the option to pass TestCase FileName depending on the needs of app.Test_Command

@dylanhitt dylanhitt changed the title WIP: Move test execution to runner file, add output.Result Move test execution to runner file, add output.Result Jun 22, 2020
@dylanhitt dylanhitt marked this pull request as ready for review June 22, 2020 00:12
@dylanhitt
Copy link
Member Author

dylanhitt commented Jun 22, 2020

@SimonBaeumer when you get a chance would you mind taking a look. My above comment explains some of my decisions. I'm hesitant to write tests. I'm not a big fan of my go/template implementation, it's kinda tough to implement the output colors and still get the benefit of using template. I will implement tests based upon how your feedback goes.

I do enjoy moving execution to runner.go as passing the the channel doesn't feel very forced anymore, as well simplifying output. We had a concern of printing the results in runtime, however with say a json implementation with an http endpoint we can delay printing/posting until all tests are finished.

Let me know what you think, Im looking forward to your feedback. I hope all is well

Cheers

@SimonBaeumer
Copy link
Member

Hey @dylanhitt!
Nice to see your pull request, it looks really good to me for a first approach!

pkg/output/template.go Outdated Show resolved Hide resolved
pkg/runtime/runner.go Show resolved Hide resolved
pkg/runtime/runtime.go Outdated Show resolved Hide resolved
pkg/runtime/runtime.go Outdated Show resolved Hide resolved
@SimonBaeumer
Copy link
Member

If you need any help on formatting issues let me know - I can help you with the annoying tasks ;)

@codeclimate
Copy link

codeclimate bot commented Jul 7, 2020

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

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

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

View more on Code Climate.

@dylanhitt
Copy link
Member Author

@SimonBaeumer I believe this ready for review

Copy link
Member

@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.

This looks really good to me!
I only requested some minor changes. 👍
A point we can discuss another time is to move the aggregation and composition of the suites to the runtime package, but atm it is not necessary in my opinion.

Would be very happy to merge this :)

pkg/runtime/runtime.go Outdated Show resolved Hide resolved
pkg/runtime/runner_test.go Show resolved Hide resolved
pkg/runtime/runner_test.go Outdated Show resolved Hide resolved
pkg/app/test_command.go Show resolved Hide resolved
pkg/app/test_command.go Outdated Show resolved Hide resolved
pkg/app/test_command.go Outdated Show resolved Hide resolved
@dylanhitt dylanhitt changed the title Move test execution to runner file, add output.Result Move test execution to runner file, refactor output Jul 9, 2020
@dylanhitt
Copy link
Member Author

@SimonBaeumer should be good to go now. I don't know if you want an update to the changelog since this is a just refactor. Unless you plan on incrementing versions. If so let me know.

@SimonBaeumer
Copy link
Member

Merge! :)

@SimonBaeumer SimonBaeumer merged commit 00f5f97 into commander-cli:master Jul 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Output package has too much responsibilities
2 participants