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

Fix!/fix certain naming issues, better patterns for multitests with parts #986

Merged
merged 8 commits into from
Oct 30, 2023
Merged

Fix!/fix certain naming issues, better patterns for multitests with parts #986

merged 8 commits into from
Oct 30, 2023

Conversation

zhenyu-ms
Copy link
Contributor

@zhenyu-ms zhenyu-ms commented Aug 24, 2023 •
edited
Loading

Spec to be discussed and updated.

Bug / Requirement Description

Clearly and concisely describe the problem.

Solution description

Describe your code changes in detail for reviewers.

Checklist:

  • Test
  • Example (both test_plan.py and .rst)
  • Documentation (API)
  • News fragment present for release notes
  • MS info leakage check
  • For new driver: driver index page
  • For new assertion: ui/pdf/std renderers, documentation
  • For new cmdline arg: documentation

@zhenyu-ms zhenyu-ms requested a review from Pyifan as a code owner August 24, 2023 08:37
@zhenyu-ms zhenyu-ms marked this pull request as draft September 22, 2023 09:58
@zhenyu-ms zhenyu-ms force-pushed the re-test-naming branch 6 times, most recently from 2375b46 to f903ca3 Compare September 27, 2023 03:19
@zhenyu-ms
a revision on test naming
2b7ca3d 
- fix tracing tests mt name output
- fix xfail tests st name matching
- fix task reassign check
- some refactor & typing fix
@zhenyu-ms
a revision on parts
52ec271 
- have consistent parts no matter filters presented
- apply certain constraints on test names
- fix a testing utility bug
@zhenyu-ms zhenyu-ms marked this pull request as ready for review October 9, 2023 10:26
@zhenyu-ms zhenyu-ms changed the title Fix!/fix certain naming issues Fix!/fix certain naming issues, better patterns for multitests with parts Oct 10, 2023
kn-ms
kn-ms requested changes Oct 11, 2023
View reviewed changes
testplan/testing/filtering.py Outdated Show resolved Hide resolved
testplan/testing/filtering.py Show resolved Hide resolved
tests/functional/testplan/runnable/interactive/test_interactive.py Show resolved Hide resolved
testplan/testing/filtering.py Outdated Show resolved Hide resolved
Pyifan
Pyifan requested changes Oct 12, 2023
View reviewed changes
Copy link
Contributor

@Pyifan Pyifan left a comment

Choose a reason for hiding this comment

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

When we change the part logic & add offset, do we need any change on merge end?

testplan/testing/filtering.py Outdated Show resolved Hide resolved
testplan/testing/filtering.py
test_ttl_part_p
)
except ValueError:
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we swallow the ValueError here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the ValueError should only come out from converting to integer call, if both of them are integers have a simple sanity check, else just pass it to fnmatch calls

testplan/testing/filtering.py Outdated Show resolved Hide resolved
testplan/testing/filtering.py
res = res and self.filter_case(case)
if not res:
return False
return True
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm not sure I like the refactored version better... the original version is more direct about the intention.

speaking of filter_levels, maybe have a mapping from level -> handler, e.g FilterLevel.Test -> filter_test, then we can avoid enumerating all levels literally.

testplan/common/report/base.py Outdated Show resolved Hide resolved
testplan/testing/multitest/suite.py Outdated Show resolved Hide resolved
doc/newsfragments/2626_changed.pattern_with_parts.rst Outdated Show resolved Hide resolved
doc/newsfragments/2626_changed.pattern_with_parts.rst Outdated Show resolved Hide resolved
@zhenyu-ms
Copy link
Contributor Author

When we change the part logic & add offset, do we need any change on merge end?

Update on dividing and merging cases could worth another PR, will revert related changes in this PR.

@zhenyu-ms
update namings
dbc502c 
revert a bad refactor as well
Pyifan
Pyifan previously approved these changes Oct 25, 2023
View reviewed changes
kn-ms
kn-ms previously approved these changes Oct 27, 2023
View reviewed changes
@zhenyu-ms zhenyu-ms dismissed stale reviews from kn-ms and Pyifan via 42328d6 October 27, 2023 08:11
@zhenyu-ms zhenyu-ms merged commit 2d258ec into morganstanley:main Oct 30, 2023
15 checks passed
@zhenyu-ms zhenyu-ms deleted the re-test-naming branch October 30, 2024 03:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pyifan Pyifan Pyifan approved these changes

@kn-ms kn-ms kn-ms left review comments

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

Successfully merging this pull request may close these issues.
3 participants
@zhenyu-ms @Pyifan @kn-ms