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

Implement option to write output to a file in ec test #1244

Merged
merged 1 commit into from
Jan 9, 2024

Conversation

mbestavros
Copy link
Member

@mbestavros mbestavros commented Dec 19, 2023

This PR adds an option to ec test to allow output to a file. Users can use the new option with ec test ... --file <file path>.

The PR utilizes new functionality available in Conftest 0.47 to write Conftest output to a file.

Resolves EC-55.

Copy link

codecov bot commented Dec 19, 2023

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (1450222) 82.43% compared to head (7935465) 82.21%.
Report is 64 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1244      +/-   ##
==========================================
- Coverage   82.43%   82.21%   -0.22%     
==========================================
  Files          72       72              
  Lines        5721     5746      +25     
==========================================
+ Hits         4716     4724       +8     
- Misses       1005     1022      +17     
Flag Coverage Δ
acceptance 65.88% <30.76%> (-0.16%) ⬇️
generative 4.31% <ø> (ø)
integration 18.10% <ø> (ø)
unit 75.85% <ø> (ø)

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

Files Coverage Δ
cmd/test/test.go 72.99% <30.76%> (-9.16%) ⬇️

cmd/test/test.go Outdated Show resolved Hide resolved
Copy link
Member

@lcarva lcarva left a comment

Choose a reason for hiding this comment

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

It's unfortunate that this part of the code base has no unit test at all. Be sure to perform some manual tests.

@simonbaird
Copy link
Member

Not sure if the previous comments were addressed or not. Is this ready for fresh reviews @mbestavros ?

@mbestavros
Copy link
Member Author

@simonbaird I wasn't able to get io.Copy() working like I expected, so I pushed another workaround. Should be good to go for another review.

Copy link
Member

@zregvart zregvart left a comment

Choose a reason for hiding this comment

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

Some nitpickery, otherwise LGTM

cmd/test/test.go Outdated
if err != nil {
return fmt.Errorf("creating output file: %w", err)
}
}
fmt.Printf("%s\n", reportOutput)
Copy link
Member

Choose a reason for hiding this comment

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

Just noticed we're not using cmd.OutOrStdout() here, we should, could be in a followup PR as this wasn't added 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 tossed it in!

cmd/test/test.go Outdated
if err != nil {
return fmt.Errorf("copying output file to stdout: %w", err)
}
fmt.Println(string(contents))
Copy link
Member

Choose a reason for hiding this comment

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

I think we want to use cmd.OutOrStdout() here

Suggested change
fmt.Println(string(contents))
fmt.Fprintln(cmd.OutOrStdout(), string(contents))

Copy link
Member Author

@mbestavros mbestavros Jan 5, 2024

Choose a reason for hiding this comment

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

@zregvart Done!

@mbestavros mbestavros force-pushed the hacbs-2519 branch 2 times, most recently from fa941d9 to 37cab1d Compare January 5, 2024 16:24
cmd/test/test.go Outdated Show resolved Hide resolved
@mbestavros mbestavros merged commit 1aa98b8 into enterprise-contract:main Jan 9, 2024
10 of 12 checks passed
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