Skip to content

chore(modelarmor): added floorsettings tests #2144

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mihirvala-crestdata
Copy link

Added floor settings tests as issue b/424365799 is fixed.

Added tests for

  • Get Folder floor settings
  • Get Organization floor settings
  • Get Project Floor settings
  • Update Folder floor settings
  • Update Organization floor settings
  • Update Project floor settings

@mihirvala-crestdata mihirvala-crestdata requested review from a team as code owners July 7, 2025 11:21
@product-auto-label product-auto-label bot added api: modelarmor Issues related to the Model Armor API. samples Issues that are directly related to samples. labels Jul 7, 2025
@harshnasitcrest
Copy link
Contributor

harshnasitcrest commented Jul 7, 2025

NOTE: Please set following environment variables required for running tests:
MA_FOLDER_ID: 695279264361
MA_ORG_ID: 951890214235

@utsav1810 utsav1810 added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jul 7, 2025
@kokoro-team kokoro-team removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jul 7, 2025
/**
* Reset project floor settings to default values
*/
protected static function resetProjectFloorSettings(): void

Choose a reason for hiding this comment

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

The code across all the methods of type reset...FloorSettings seems redundant. Can you please extract the common code in a separate method & use it for Project/Folder/Organization.

Copy link
Author

Choose a reason for hiding this comment

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

Updated logic for resetting floor settings to remove redundant logic.

Copy link
Contributor

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

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

NOTE: Please set following environment variables required for running tests: MA_FOLDER_ID: 695279264361 MA_ORG_ID: 951890214235

Which project are these credentials for? I used these for the standard test project (php-docs-samples-kokoro) and was unable to get them to work.

Comment on lines 104 to 105
self::$organizationId = getenv('MA_ORG_ID');
self::$folderId = getenv('MA_FOLDER_ID');
Copy link
Contributor

Choose a reason for hiding this comment

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

let's use requireEnv for these, that way people running the tests will know they're required:

Suggested change
self::$organizationId = getenv('MA_ORG_ID');
self::$folderId = getenv('MA_FOLDER_ID');
self::$organizationId = self::requireEnv('MA_ORG_ID');
self::$folderId = self::requireEnv('MA_FOLDER_ID');

Copy link
Author

Choose a reason for hiding this comment

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

updated, thanks.

@mihirvala-crestdata
Copy link
Author

NOTE: Please set following environment variables required for running tests: MA_FOLDER_ID: 695279264361 MA_ORG_ID: 951890214235

Which project are these credentials for? I used these for the standard test project (php-docs-samples-kokoro) and was unable to get them to work.

Did not get this.
We used the same Folder ID and Organization ID for testing model armor snippets across all languages.

cc: @bhardwaj-prerna @utsav1810

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: modelarmor Issues related to the Model Armor API. samples Issues that are directly related to samples.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants