Allow for creating reports from other reports #174
Conversation
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
left a comment
wash-amzn
left a comment
There was a problem hiding this comment.
What is the behavior with tar.gz'ed reports versus extracted ones? Is it consistent with tar.gz'ed vs extracted 'aperf record' outputs?
| 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.
tests/test_aperf.rs
Outdated
| 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.
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. |
2 participants
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.