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.

Sign up for GitHub

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

Jump to bottom

Reopen "Change test_name's formart to file_path#class#func" #12

Merged
merged 5 commits into from
Oct 28, 2020
Merged

Reopen "Change test_name's formart to file_path#class#func" #12

merged 5 commits into from
Oct 28, 2020

Conversation

ninjinkun
Copy link
Contributor

@ninjinkun ninjinkun commented Oct 26, 2020 •
edited
Loading

@shuheiktgw I reopen #10. Sorry for confusing you.

@ninjinkun
Revert "Merge pull request #11 from launchableinc/revert-10-fix-test-…
d20a68c 
…name"

This reverts commit e70f824, reversing
changes made to 4db10e7.
@ninjinkun ninjinkun requested a review from shuheiktgw October 26, 2020 01:43
@ninjinkun ninjinkun changed the title Change test_name's formart to file_path#class#func Reopen Change test_name's formart to file_path#class#func https://github.com/launchableinc/nose-launchable/pull/10 Oct 26, 2020
@ninjinkun ninjinkun changed the title Reopen Change test_name's formart to file_path#class#func https://github.com/launchableinc/nose-launchable/pull/10 Reopen Change test_name's formart to file_path#class#func #10 Oct 26, 2020
@ninjinkun ninjinkun changed the title Reopen Change test_name's formart to file_path#class#func #10 Reopen "Change test_name's formart to file_path#class#func" Oct 26, 2020
@shuheiktgw
Copy link
Contributor

@ninjinkun #10 I left a few comments on the PR! 🙏

@ninjinkun
Copy link
Contributor Author

@shuheiktgw I continue #10 discussion on this PR.

#10 (comment) Yes. I worried about the same thing. Developers change directories not frequently but sometimes they do. Although we can add an option to specify the root directory, I worried they can't continue to maintain the relationship between the root directory and test directories.

@ninjinkun
Copy link
Contributor Author

I'm researching how Git solves this consistency problem

@ninjinkun
Copy link
Contributor Author

#10 (comment) I fixed this issue.

@ninjinkun
Copy link
Contributor Author

ninjinkun commented Oct 26, 2020 •
edited
Loading

#10 (comment) For this test generator problem, I plan to aggregate all generated tests' results. For example, test_evens(0, 0) is succeeded, test_evens(1, 1) is failed, test_evens(2, 2) is succeeded, this plugin sends a test_evens failed event. As a negative point, It needs waiting for all tests are done because we cannot know when all generated tests are done.
@shuheiktgw How do you think about this idea?

@shuheiktgw
Copy link
Contributor

@ninjinkun Hmm, test.id() seems to have a generated test name can we use it to identify a test name?

(Pdb) test.id()
'tests.dir3.test1.test_evens(0, 0)'

@ninjinkun
Copy link
Contributor Author

@shuheiktgw It works. However, I worried about generating tests with random parameters. If we use test.id(), we need to group these tests on the server.

@ninjinkun
Copy link
Contributor Author

@shuheiktgw But at this time, we need to go forward. I'm going to use the name with parameters such as test_evens(0, 0).

shuheiktgw
shuheiktgw reviewed Oct 27, 2020
View reviewed changes
launchable/plugin.py Outdated
def get_test_name(t):
if isinstance(t.context, ModuleType):
return t.context.__name__
file_path, module, = test_address(t.test)
Copy link
Contributor

@shuheiktgw shuheiktgw Oct 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I guess a function name is missing...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed it!

shuheiktgw
shuheiktgw reviewed Oct 27, 2020
View reviewed changes
launchable/plugin.py Outdated
@@ -92,7 +92,8 @@ def afterTest(self, test):

@protect
def addError(self, test, err, capt=None):
self._addResult(test, CaseEvent.TEAT_FAILED, self._uploader.enqueue_failure)
if err is not ImportError:
Copy link
Contributor

@shuheiktgw shuheiktgw Oct 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It seems if a non-existing test is specified, err is tuple like: (<class 'ValueError'>, ValueError('No such test aaa'), <traceback object at 0x1044a2040>). You can check this by running nosetests tests/dir3/test1.py:aaa -v --nocapture --nologcapture --launchable --launchable-build-number 1 in the rocket-car repo if you need!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed to handle ValueError type

@ninjinkun
Copy link
Contributor Author

@shuheiktgw My apologies, my local testing has failed because of the wrong auth token. I'm going to fix my local, then fix my code.

@ninjinkun
Copy link
Contributor Author

ninjinkun commented Oct 27, 2020 •
edited
Loading

@shuheiktgw I fixed them

shuheiktgw
shuheiktgw approved these changes Oct 28, 2020
View reviewed changes
Copy link
Contributor

@shuheiktgw shuheiktgw left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@ninjinkun ninjinkun merged commit ec27a93 into main Oct 28, 2020
@ninjinkun ninjinkun deleted the revert-revert-fix-test-name branch October 28, 2020 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shuheiktgw shuheiktgw shuheiktgw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ninjinkun @shuheiktgw