Allow for creating reports from other reports #174
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The APerf reports contain the data archives. Use these if a report is passed in to the aperf report sub-command. The aperf report command will fail if a report is given as an arg and the report name is omitted. Also, move to using PathBuf instead of String for path.
wash-amzn
reviewed
May 15, 2024
Comment on lines
+89
to
+92
| if dir.join("index.css").exists() | ||
| && dir.join("index.html").exists() | ||
| && dir.join("index.js").exists() | ||
| && dir.join("data").exists() |
There was a problem hiding this comment.
Do these checks really matter enough to be worth this breaking if we were to, for example, rename the index.js file in reports?
There was a problem hiding this comment.
We're adding integration tests and the directory structure is checked for in there.
Add an integration test for aperf record and aperf report. Integration tests are located under tests/. The integration test for aperf report also checks the structure of the report in both the directory and the tarball.
wash-amzn
reviewed
May 22, 2024
tests/test_aperf.rs
Outdated
Comment on lines
65
to
68
| assert!(paths.contains(&PathBuf::from("test_report/index.html"))); | ||
| assert!(paths.contains(&PathBuf::from("test_report/index.css"))); | ||
| assert!(paths.contains(&PathBuf::from("test_report/index.js"))); | ||
| assert!(paths.contains(&PathBuf::from("test_report/data/archive"))); |
There was a problem hiding this comment.
In the case of failures (including these assertion failures), files are going to be leaked. Can you look into passing some trait object to handle file/io, or otherwise mock this such that files aren't being created?
There was a problem hiding this comment.
We're using the tempdir crate along with a custom test harness to make sure we clean up after ourselves.
wash-amzn
approved these changes
May 28, 2024
For integration testing, we need a test harness which can setup the temp directory. All the files generated as part of the tests should be located in this dir. The test harness will be able to catch assert failures and re-raise them after cleaning up the temp directory. This requires all integration tests run as closures in run_test().
|
Fixed clippy and fmt warnings. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The APerf reports contain the data archives. Use these if a report is passed in to the aperf report sub-command. The aperf report command will fail if a report is given as an arg and the report name is omitted. Also, move to using PathBuf instead of String for path.
Testing:
Tested by generating reports with:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.