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

Support suffix for test classes when using suite. #467

Merged
merged 4 commits into from
Jul 18, 2018
Merged

Support suffix for test classes when using suite. #467

merged 4 commits into from
Jul 18, 2018

Conversation

xpconanfan
Copy link
Collaborator

@xpconanfan xpconanfan commented Jun 25, 2018

So the same test class can be run with different configs and reported differently in the same suite.

Aliases are only possible in a suite as they are set when calling add_test_class.


This change is Reviewable

So the same test class can be run with different configs and reported
differently in the same suite.

Aliases are only possible in a suite as they are set when calling
`add_test_class`.
@xpconanfan xpconanfan added this to the Mobly Release 1.7.4 milestone Jun 25, 2018
@xpconanfan xpconanfan self-assigned this Jun 25, 2018
@winterfroststrom
Copy link
Contributor

Review status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @xpconanfan, @winterfroststrom, and @k2fong)


tests/mobly/test_runner_test.py, line 292 at r1 (raw file):

        record1 = tr.results.executed[0]
        record2 = tr.results.executed[1]
        self.assertEqual(record1.test_class, 'IntegrationTest-FirstConfig')

I don't see a check for the base functionality, so add a new unit test or add asserts to test_run_two_test_classes


Comments from Reviewable

@xpconanfan
Copy link
Collaborator Author

Review status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @xpconanfan, @winterfroststrom, and @k2fong)


tests/mobly/test_runner_test.py, line 292 at r1 (raw file):

Previously, winterfroststrom wrote…

I don't see a check for the base functionality, so add a new unit test or add asserts to test_run_two_test_classes

what is the "base functionality"?


Comments from Reviewable

@winterfroststrom
Copy link
Contributor

Review status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @winterfroststrom and @k2fong)


tests/mobly/test_runner_test.py, line 292 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

what is the "base functionality"?

that the tag is set to the class name?


Comments from Reviewable

@winterfroststrom
Copy link
Contributor

Review status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @winterfroststrom and @k2fong)


tests/mobly/test_runner_test.py, line 292 at r1 (raw file):

Previously, winterfroststrom wrote…

that the tag is set to the class name?

err, I should say that the record.test_class is the class name


Comments from Reviewable

@xpconanfan
Copy link
Collaborator Author

Review status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @xpconanfan, @winterfroststrom, and @k2fong)


tests/mobly/test_runner_test.py, line 292 at r1 (raw file):

Previously, winterfroststrom wrote…

err, I should say that the record.test_class is the class name

isn't that what this assert equal checks?


Comments from Reviewable

@winterfroststrom
Copy link
Contributor

Review status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @winterfroststrom and @k2fong)


tests/mobly/test_runner_test.py, line 292 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

isn't that what this assert equal checks?

This assert checks that you can change the default functionality to set a custom class alias, it does not check that the original class name is as expected


Comments from Reviewable

@k2fong
Copy link
Contributor

k2fong commented Jun 26, 2018

Review status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @xpconanfan, @winterfroststrom, and @k2fong)


tests/mobly/test_runner_test.py, line 293 at r1 (raw file):

IntegrationTest-SecondConfig

Will the original test class name be preserve somewhere in the output? or will it just be the alias if specified?


Comments from Reviewable

@xpconanfan
Copy link
Collaborator Author

Review status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @xpconanfan, @winterfroststrom, and @k2fong)


tests/mobly/test_runner_test.py, line 293 at r1 (raw file):

Previously, k2fong wrote…
IntegrationTest-SecondConfig

Will the original test class name be preserve somewhere in the output? or will it just be the alias if specified?

Just the alias. It changes how the class is referred to.


Comments from Reviewable

@xpconanfan
Copy link
Collaborator Author

Review status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @winterfroststrom and @k2fong)


tests/mobly/test_runner_test.py, line 292 at r1 (raw file):

Previously, winterfroststrom wrote…

This assert checks that you can change the default functionality to set a custom class alias, it does not check that the original class name is as expected

Done.


Comments from Reviewable

@winterfroststrom
Copy link
Contributor

:lgtm:


Review status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @k2fong)


Comments from Reviewable

@winterfroststrom
Copy link
Contributor

Review status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @k2fong and @xpconanfan)


tests/mobly/test_runner_test.py, line 266 at r2 (raw file):

        record2 = tr.results.executed[1]
        self.assertEqual(record1.test_class, 'IntegrationTest')
        self.assertEqual(record1.test_class, 'IntegrationTest')

Wait, actually this check seems wrong

Additionally, Integration2Test is added first


Comments from Reviewable

@k2fong
Copy link
Contributor

k2fong commented Jun 26, 2018

Review status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @k2fong and @xpconanfan)


tests/mobly/test_runner_test.py, line 293 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

Just the alias. It changes how the class is referred to.

Will it make sense to preserve the original class name and append (or some reader friendly way) the extra string from the user to differentiate the different tests instead of replacing?


Comments from Reviewable

@xpconanfan
Copy link
Collaborator Author

Review status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @xpconanfan and @k2fong)


tests/mobly/test_runner_test.py, line 293 at r1 (raw file):

Previously, k2fong wrote…

Will it make sense to preserve the original class name and append (or some reader friendly way) the extra string from the user to differentiate the different tests instead of replacing?

It sounds good, but I can't think of a use case.
Also it requires adding field in the record obj, so I'm not sure.
What do you think?


Comments from Reviewable

@k2fong
Copy link
Contributor

k2fong commented Jun 26, 2018

Review status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @k2fong and @xpconanfan)


tests/mobly/test_runner_test.py, line 293 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

It sounds good, but I can't think of a use case.
Also it requires adding field in the record obj, so I'm not sure.
What do you think?

The use case will be same as this pull request. On a different system I have seem them do this

IntegrationTest [FirstConfig]
IntegrationTest [SecondConfig]

user can pass in what goes into the bracket.

If there might be worry about adding field, couldn't the new appended name just replace the default just like you have done for alias?


Comments from Reviewable

@winterfroststrom
Copy link
Contributor

Review status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @xpconanfan and @k2fong)


tests/mobly/test_runner_test.py, line 293 at r1 (raw file):

Previously, k2fong wrote…

The use case will be same as this pull request. On a different system I have seem them do this

IntegrationTest [FirstConfig]
IntegrationTest [SecondConfig]

user can pass in what goes into the bracket.

If there might be worry about adding field, couldn't the new appended name just replace the default just like you have done for alias?

why is it better for mobly to do this rather than the user?
Since they can get the class name and set the logged name arbitrarily, so they could just implement it like that?


Comments from Reviewable

@k2fong
Copy link
Contributor

k2fong commented Jun 27, 2018

Review status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @xpconanfan and @k2fong)


tests/mobly/test_runner_test.py, line 293 at r1 (raw file):

Previously, winterfroststrom wrote…

why is it better for mobly to do this rather than the user?
Since they can get the class name and set the logged name arbitrarily, so they could just implement it like that?

agree, user can do it them self, but having mobly do this can enforce some consistency across all users.


Comments from Reviewable

@winterfroststrom
Copy link
Contributor

Review status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @xpconanfan and @k2fong)


tests/mobly/test_runner_test.py, line 293 at r1 (raw file):

Previously, k2fong wrote…

agree, user can do it them self, but having mobly do this can enforce some consistency across all users.

Hmm, there's question is what the consistency buys us.

If we ever want to add some analysis/report generation stuff for example,
then consistency could be helpful as it can establiss invariants/assumptions.

I think I could go either way on this.


Comments from Reviewable

Copy link
Collaborator Author

@xpconanfan xpconanfan left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @k2fong)


tests/mobly/test_runner_test.py, line 293 at r1 (raw file):

Previously, winterfroststrom wrote…

Hmm, there's question is what the consistency buys us.

If we ever want to add some analysis/report generation stuff for example,
then consistency could be helpful as it can establiss invariants/assumptions.

I think I could go either way on this.

Done.

Copy link
Contributor

@k2fong k2fong left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @k2fong)

@xpconanfan xpconanfan merged commit 6f084fc into master Jul 18, 2018
@xpconanfan xpconanfan deleted the tr branch July 18, 2018 00:17
@xpconanfan xpconanfan changed the title Support alias for test classes when using suite. Support suffix for test classes when using suite. Jul 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants