Added documentation and test coverage. #10035
Conversation
|
@shahmishal @gottesmm Please review 🙏 |
|
I have force-pushed fix for lint complaints. |
compare_perf_test.py is now covered with unit tests and public methods are documented in the implementation. Minor refactoring to better conform to Python conventions: * classes declared in new style * proper private method prefix of single underscore * replacing map with list comprehension where it was clearer Unit test are executed as part of validation-test. .gitignore was modified to ignore .coverage and htmlcov artifacts generated by the coverage.py package
palimondo
force-pushed
the
got-you-covered
branch
from
322ace1 to
49ddd96
Compare
|
@swift-ci please python lint |
Coverage at 66% according to coveragy.py
|
@swift-ci please smoke test |
| from Swift Benchmark Suite as reported by test driver (Benchmark_O, | ||
| Benchmark_Onone, Benchmark_Ounchecked or Benchmark_Driver). | ||
|
|
||
| It depends on the log format emmited by the test driver in the form: |
There was a problem hiding this comment.
Typo: the correct spelling is "emitted"
| It depends on the log format emmited by the test driver in the form: | ||
| #,TEST,SAMPLES,MIN(μs),MAX(μs),MEAN(μs),SD(μs),MEDIAN(μs),MAX_RSS(B) | ||
|
|
||
| The last column, MAX_RSS, is emmited only for runs instrumented by the |
| @@ -21,8 +21,22 @@ | |||
| from math import sqrt | |||
|
|
|||
|
|
|||
| class PerformanceTestResult: | |||
| class PerformanceTestResult(object): | |||
| """PerformanceTestResult holds results from executing individual benchmark | |||
There was a problem hiding this comment.
Missing articles: "from executing an individual benchmark from the Swift Benchmark Suite..."
Coverage at 87% according to coveragy.py Also fixed spelling errors in documentation.
|
@shahmishal Sorry for pushing another change while the smoke test was running, but I'd hate to open extra PR just to fix the grammar nits @xwu brought up. I had a couple more test ready, so I slipped those in, too. Please re-🚬 I'm going to sleep now, so the branch should be stable for the next few hours 😇 |
|
@swift-ci please smoke test |
|
@shahmishal @gottesmm @dabrahams @eeckstein @atrick Can somebody please review and merge this? Otherwise I can't help myself and will keep tinkering with it... |
|
@CodaFi @natecook1000 Could you? ^^ |
|
@swift-ci Please test and merge |
|
@palimondo This really should be run through the full validation suite, but if that passes, it will merge. Thanks for your hard work on this! |
Coverage at 99% according to coverage.py * `compare_perf_tests.py` now always outputs the same format to stdout as is written to `--output` file * Added integration test for the main() function * Added tests for console output (and suppressed it leaking during testing) * Fixed file name in test’s file header
|
OMG, Sorry @dabrahams, I haven't seen your response (GitHub didn't update until I refreshed browser) and I pushed a final commit. Could you please re-run the CI? |
|
Ah, I'm so clumsy... fixing everything in 1000 steps. I would have force pushed, but it seems to mess with CI, requiring it to run twice, so I added two more commits instead. 🤦♂️ |
|
@swift-ci Please test and merge |
|
@natecook1000 You can test, but not merge? |
|
@practicalswift Any idea why this didn't merge after passing the test? |
|
@shahmishal Thanks for the merge! |
compare_perf_test.py is now covered with unit tests and public methods are documented in the implementation.
Minor refactoring to better conform to Python conventions:
Unit test are executed as part of validation-test.
.gitignore was modified to ignore
.coverageandhtmlcovartifacts generated by thecoverage.pypackage