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

post_run: add report data to 'Results' object/class #578

Merged
merged 1 commit into from
Nov 16, 2019

Conversation

umarcor
Copy link
Member

@umarcor umarcor commented Oct 28, 2019

Close #445
Close #478
Close #574

In this PR, parameters output_path and report are passed from main to the Results object built and passed to post_run callbacks. Instead of exposing the TestReport object, as suggested in #445, a subset of info is extracted for each test: name, status, time and path. The path is relative to join(output_path, 'test_output').

Usage example:

def post_func(results):
    print(results.output_path)
    for key, val in results.get_report().items():
        print(key , val)

vu.main(post_run=post_func)

@felixn, you can build the path to each test log as follows: join(results.output_path, 'test_output', val.path, 'output.txt'). Alternatively: join(results.output_path, 'test_output', results.get_report().items()[<test_name>], 'output.txt').

/cc @richjyoung

@umarcor umarcor force-pushed the feat-results-report branch from 74364ba to 07d9482 Compare October 28, 2019 03:48
"""
return {
test.name: {
"status": test._status.name, # pylint: disable=protected-access
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a method to_dict to test instead of using private access

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 added it now. However, private access is required for _test_results_in_order(). Should I add to_dict to TestReport too?

vunit/ui/results.py Outdated Show resolved Hide resolved
vunit/ui/results.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@kraigher kraigher left a comment

Choose a reason for hiding this comment

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

Also there are no new tests added for this.

vunit/test_report.py Outdated Show resolved Hide resolved
vunit/ui/results.py Outdated Show resolved Hide resolved
@umarcor umarcor force-pushed the feat-results-report branch from 3df51b4 to 19441a8 Compare November 6, 2019 07:59
@umarcor
Copy link
Member Author

umarcor commented Nov 6, 2019

Also there are no new tests added for this.

I didn't find tests in vunit/test/unit for merge_coverage. Should I create a new vunit/test/unit/test_results.py file? Or can I add them to vunit/test/unit/test_test_report.py?

EDIT

I now added a test named test_dict_report_with_all_passed_tests to test_test_report.py. Let me know what you think.

@umarcor umarcor force-pushed the feat-results-report branch 2 times, most recently from 43e5010 to 0ab86fd Compare November 6, 2019 09:06
@kraigher
Copy link
Collaborator

kraigher commented Nov 7, 2019

I think we should not expose dicts for individual test results on the public api since it is harder to change without breaking in the future. Also see my other line comments.

@umarcor umarcor force-pushed the feat-results-report branch 2 times, most recently from 0fe1b3d to 93fae03 Compare November 7, 2019 04:38
@umarcor
Copy link
Member Author

umarcor commented Nov 12, 2019

I think we should not expose dicts for individual test results on the public api since it is harder to change without breaking in the future.

I added and used class ReportResult now. Let me know if that's what you meant.

Also see my other line comments.

I don't see any other comments. Maybe you have some unfinished review?

@umarcor umarcor force-pushed the feat-results-report branch from 4804629 to 54964dd Compare November 12, 2019 05:01
@umarcor umarcor mentioned this pull request Nov 12, 2019
1 task
vunit/ui/results.py Outdated Show resolved Hide resolved
vunit/ui/results.py Outdated Show resolved Hide resolved
vunit/ui/results.py Show resolved Hide resolved
vunit/ui/results.py Outdated Show resolved Hide resolved
@umarcor umarcor force-pushed the feat-results-report branch 2 times, most recently from 5370dd0 to 80f2797 Compare November 13, 2019 06:10
vunit/ui/results.py Outdated Show resolved Hide resolved
* add test_dict_report_with_all_passed_tests
* add classes Report and TestResult to vunit.ui.results
@umarcor umarcor force-pushed the feat-results-report branch from 80f2797 to 3d32dbf Compare November 14, 2019 05:01
@umarcor umarcor requested a review from kraigher November 16, 2019 09:31
@eine eine merged commit d1dfdd9 into VUnit:master Nov 16, 2019
@umarcor umarcor deleted the feat-results-report branch November 17, 2019 01:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants