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

refactor: make e2e tests use terramate binary #145

Merged
merged 15 commits into from
Jan 7, 2022

Conversation

i4ki
Copy link
Contributor

@i4ki i4ki commented Jan 6, 2022

In addition to the test refactor, the cli logging was changed to fatal in case of errors instead of returning it to main because this change makes it impossible to check using the sentinel errors and then there's no benefit for keeping the Fatal() in the main() function, far away from when it happens. So now all the cli package code can be moved to main package, but we can do this in a separate PR.

@codecov-commenter
Copy link

codecov-commenter commented Jan 6, 2022

Codecov Report

Merging #145 (32d6f05) into main (1a67cb2) will decrease coverage by 21.16%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main     #145       +/-   ##
===========================================
- Coverage   76.51%   55.34%   -21.17%     
===========================================
  Files          29       29               
  Lines        3738     3796       +58     
===========================================
- Hits         2860     2101      -759     
- Misses        671     1548      +877     
+ Partials      207      147       -60     
Flag Coverage Δ
tests 55.34% <0.00%> (-21.17%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/terramate/cli/cli.go 0.00% <0.00%> (-70.43%) ⬇️
cmd/terramate/main.go 0.00% <0.00%> (ø)
order.go 0.00% <0.00%> (-71.88%) ⬇️
run.go 0.00% <0.00%> (-64.71%) ⬇️
project/project.go 36.36% <0.00%> (-45.46%) ⬇️
init.go 26.92% <0.00%> (-30.77%) ⬇️
config/config.go 43.13% <0.00%> (-29.42%) ⬇️
stack/loader.go 52.35% <0.00%> (-28.78%) ⬇️
stack/stack.go 63.63% <0.00%> (-22.73%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a67cb2...32d6f05. Read the comment docs.

@i4ki i4ki requested review from katcipis and kassianh January 6, 2022 23:28
@i4ki
Copy link
Contributor Author

i4ki commented Jan 6, 2022

@katcipis look at the coverage stats =(

The go build does not support a -cover flag, then searching for workarounds I found this:
golang/go#33463 (comment)

But not sure if supports passing args... very ugly and probably also will need a "coverage merge magic".
Ideas?

@kassianh
Copy link
Contributor

kassianh commented Jan 6, 2022

Looks fine to me but it's also a bit beyond me..

@katcipis
Copy link
Contributor

katcipis commented Jan 7, 2022

@katcipis look at the coverage stats =(

The go build does not support a -cover flag, then searching for workarounds I found this: golang/go#33463 (comment)

But not sure if supports passing args... very ugly and probably also will need a "coverage merge magic". Ideas?

It is not awesome but I'm OK with just letting it drop for now (the coverage), we already plan on extracting tests that are unrelated to cli stuff. Then for some cli dependent logic we could also extract it in a way that can be tested more isolated, leaving the e2e tests for very basic full integration (which still drops coverage maybe, but not as much).

Or we can go with the hack, but it is so very hacky 😆

katcipis
katcipis previously approved these changes Jan 7, 2022
Copy link
Contributor

@katcipis katcipis left a comment

Choose a reason for hiding this comment

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

LGTM + some suggestions on how to improve the logging fatal calls, but can also be done on another PR.

cmd/terramate/cli/cli.go Outdated Show resolved Hide resolved
cmd/terramate/cli/cli.go Outdated Show resolved Hide resolved
cmd/terramate/cli/cli.go Outdated Show resolved Hide resolved
cmd/terramate/cli/cli.go Outdated Show resolved Hide resolved
cmd/terramate/cli/cli.go Outdated Show resolved Hide resolved
cmd/terramate/cli/cli.go Outdated Show resolved Hide resolved
cmd/terramate/cli/cli.go Outdated Show resolved Hide resolved
cmd/terramate/cli/cli.go Outdated Show resolved Hide resolved
chdir string
}

type runResult struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
type runResult struct {
type execResult struct {

Thinking if "exec" instead of "run" won't be more clear now. (run is not bad ...so just wondering).

Status int
}

type runExpected struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
type runExpected struct {
type execExpected struct {

i4ki and others added 2 commits January 7, 2022 10:23
Co-authored-by: Tiago Cesar Katcipis <tiagokatcipis@gmail.com>
@i4ki i4ki requested a review from katcipis January 7, 2022 10:28
katcipis
katcipis previously approved these changes Jan 7, 2022
Copy link
Contributor

@katcipis katcipis left a comment

Choose a reason for hiding this comment

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

Thanks man 💯

@i4ki i4ki merged commit 32995b8 into main Jan 7, 2022
@i4ki i4ki deleted the i4k-refactor-tests-exec-binary branch January 7, 2022 12:50
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.

4 participants