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 platform names as string constants and reduce redundancy in unit tests #667

Merged
merged 4 commits into from
Nov 29, 2023
Merged

Fix platform names as string constants and reduce redundancy in unit tests #667

merged 4 commits into from
Nov 29, 2023

Conversation

parthosa
Copy link
Collaborator

@parthosa parthosa commented Nov 20, 2023 •
edited
Loading

Fixes #666. This PR improves robustness by removing hardcoded string values for platform names.

Changes:

  1. Replace platform names as string values by constants defined in Platform.scala.
  2. Define a PlatformNames.DEFAULT that should be used as default platform for all cases.
  3. Reduce redundancy in unit tests by using loops instead of duplicate tests for each platform.

@parthosa parthosa self-assigned this Nov 20, 2023
@parthosa parthosa added the tools label Nov 20, 2023
@parthosa
Fix typo in tests
b94975c 
Signed-off-by: Partho Sarthi <[email protected]>
nartal1
nartal1 previously approved these changes Nov 21, 2023
View reviewed changes
Copy link
Collaborator

@nartal1 nartal1 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @parthosa for refactoring the code and updating the tests. This is much cleaner.

cindyyuanjiang
cindyyuanjiang previously approved these changes Nov 22, 2023
View reviewed changes
Copy link
Collaborator

@cindyyuanjiang cindyyuanjiang left a comment

Choose a reason for hiding this comment

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

minor nits, otherwise LGTM, thanks @parthosa!

@parthosa parthosa dismissed stale reviews from cindyyuanjiang and nartal1 via 8533b52 November 28, 2023 00:44
cindyyuanjiang
cindyyuanjiang approved these changes Nov 29, 2023
View reviewed changes
Copy link
Collaborator

@cindyyuanjiang cindyyuanjiang left a comment

Choose a reason for hiding this comment

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

Thanks @parthosa!

@parthosa parthosa merged commit 6d2fb4e into NVIDIA:dev Nov 29, 2023
8 checks passed
@parthosa parthosa deleted the spark-rapids-tools-666 branch November 29, 2023 00:49
@parthosa parthosa added the bug Something isn't working label Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cindyyuanjiang cindyyuanjiang cindyyuanjiang approved these changes

@nartal1 nartal1 nartal1 approved these changes

@mattahrens mattahrens Awaiting requested review from mattahrens

Assignees

@parthosa parthosa

Labels
bug Something isn't working tools
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] Revise string names of Platform
3 participants
@parthosa @cindyyuanjiang @nartal1