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

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

Merged
merged 4 commits into from
Nov 29, 2023

Conversation

parthosa
Copy link
Collaborator

@parthosa parthosa commented Nov 20, 2023

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 core_tools Scope the core module (scala) label Nov 20, 2023
Signed-off-by: Partho Sarthi <[email protected]>
@parthosa parthosa force-pushed the spark-rapids-tools-666 branch from a30fc60 to b94975c Compare November 21, 2023 17:51
nartal1
nartal1 previously approved these changes Nov 21, 2023
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
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!

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
Labels
bug Something isn't working core_tools Scope the core module (scala)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] Revise string names of Platform
3 participants