This repository was archived by the owner on Feb 20, 2026. It is now read-only.
test: Add tests that describe how save should encode excludeFromIndexes for a complex case#1263
Merged
danieljbruce merged 34 commits into1242-to-main-2from Aug 28, 2024
Merged
Conversation
product-auto-label
bot
added
size: l
This is in excludeFromIndexes - replace them with literals.
Matches the single case
encoding problem
daniel-sanche
approved these changes
Aug 26, 2024
danieljbruce
added a commit
that referenced
this pull request
Aug 28, 2024
… of nested fields (#1266) * test: Restrict types inside the save function (#1264) * Add interfaces that restrict values passed to save * Add interfaces for save We need help from the compiler so these interfaces will be useful for determining the shape of the data. * Modify current interfaces to make a closer match The interfaces should match the actual data types that get passed in. * key and data in PrepareEntityObjectResponse option These properties are actually optional * Document all the interfaces added for the save fn * Add two tests showing name/value should be provide * Make the SaveNonArrayData more specific. save doesn’t encode strings or ints or any values that don’t have keys properly so we should restrict the interface to what it does include. * Add header * Test should be more flexible * Add a class for elements with ToString * Define the toString class inline * Eliminate links to points in code * test: Add tests that describe how save should encode excludeFromIndexes for a complex case (#1263) * Write a sample test for what save output should be * Add a test for a bunch of complex excludeFromIndex * Change the replace function so it works everywhere * Modify the expected value Make it contain longStringArray * Correct the test for evaluating arrays * Break up the entityToEntityProto into separate par * Fix the tests to work with new addExcludeFromIndex * Revert "Break up the entityToEntityProto into separate par" This reverts commit 60dabd7. * Revert "Fix the tests to work with new addExcludeFromIndex" This reverts commit ba4e82b. * Remove source code changes * Skip the test that looks at arrays * Remove some of the wildcard indexes This is in excludeFromIndexes - replace them with literals. * Remove another boolean value from excludeFromIndex * Eliminate duplicate expectedMutations * Eliminate duplicate runTest function * Begin to set up the async tests * Pack all tests into the async framework * Delete tests addressed by async * Get rid of test functions and inline everything * Add comment * Add comments describing each test case * Remove only * Add another test looks at name/value not in array * Add a test for excludeLarge properties and name/va * Fix the test so that the array case matches Matches the single case * should pass the right properties for an array * Rename the test * Change name to entityName * Add two tests that capture the array encoding encoding problem * Change expected output of arrays * Remove only * Add 2 more tests to ensure behaviour is preserved * Correct test - it should apply excludeFromIndexes * Update the test for the nested value * fix: Handle excludeLarge properties for arrays properly (#1265) * Write a sample test for what save output should be * Add a test for a bunch of complex excludeFromIndex * Change the replace function so it works everywhere * Modify the expected value Make it contain longStringArray * Correct the test for evaluating arrays * Break up the entityToEntityProto into separate par * Fix the tests to work with new addExcludeFromIndex * Break up the entityToEntityProto into separate par * Fix the tests to work with new addExcludeFromIndex * Revert "Break up the entityToEntityProto into separate par" This reverts commit 60dabd7. * Revert "Fix the tests to work with new addExcludeFromIndex" This reverts commit ba4e82b. * Remove source code changes * Skip the test that looks at arrays * Remove some of the wildcard indexes This is in excludeFromIndexes - replace them with literals. * Remove another boolean value from excludeFromIndex * Eliminate duplicate expectedMutations * Eliminate duplicate runTest function * Begin to set up the async tests * Pack all tests into the async framework * Delete tests addressed by async * Get rid of test functions and inline everything * Add comment * Add comments describing each test case * Remove only * Add another test looks at name/value not in array * Add a test for excludeLarge properties and name/va * Fix the test so that the array case matches Matches the single case * should pass the right properties for an array * Rename the test * Change name to entityName * Add two tests that capture the array encoding encoding problem * Change expected output of arrays * Remove only * Add 2 more tests to ensure behaviour is preserved * Make argument more specific for excludeFromIndexes * Use excludeLargeProperties for array/non-array cas * Fix the test to include entity proto * Correct mistakes in initial implementation * Don’t skip any of the tests * Remove only * Add check for excludeLargeProperties * Remove TODO * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --------- Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
As part of the broader effort to solve the bug in this document properly this PR will add tests for the save function that ensure the right data is passed to the Gapic layer. Some tests are skipped as they fail because the issue described in #1242 hasn't been solved yet. When these the issue is solved these tests should be passing.
Changes:
10 tests are added - 2 of which are skipped and need source code changes to address
Four simple tests are added to demonstrate the differences between saving array data and saving non-array data:
should pass the right properties for a simple name/value pairis added to demonstrate how non-array data is currently save for data with name and value properties.should pass the right properties for a simple name/value pair in an arrayis added to demonstrate that when the data from the previous test is wrapped in an array that thenameandvalueproperties take on a special meaning for how the data is encoded.should position excludeFromIndexes in the right place when provided at the top levelensures that for arrays whenexcludeFromIndexes: trueis provided in the right place at the top level that it is respected.should pass the right properties for a simple name/value pair in an array with excludeFromIndexes listensures thatthe
excludeFromIndexeslist is used properly to addexcludeFromIndexes: truein the right places.Two more complex tests are added for non name/value pair data that compare setting
excludeLargePropertiesto true and providing anexcludeFromIndexeslist:should pass the right request with a bunch of large properties excluded- passes - Sample input data, and excludeFromIndexes data was taken from this test and passed into the save function withexcludeLargePropertiesnot set. When thisdata/excludeFromIndexesis passed intosavethe mutations that get passed to the Gapic layer are checked for correctness against anexpectedMutationsobject. It looks like this encoding works correctly.should pass the right properties for an object with excludeLargeProperties- passes - the same data fromshould pass the right request with a bunch of large properties excludedis used, but no excludeFromIndexes data is provided. Instead, theexcludeLargePropertiesflag is set to true so anexcludeFromIndexeslist is generated. The mutations passed to the Gapic layer are compared with anexpectedMutationsobject which correctly hasexcludeFromIndexes: truebeside all the values containing a large string and the test passes as expected. The previous test has the sameexpectedMutations.A test is added that matches the code from the Github issue:
should pass the right request with a nested field- skipped - This test directly evaluates the code the user says is failing in Unable to exclude large indexes #1242 and we need source code changes for this test to pass.3 more complex tests are added for name/value pair combinations demonstrating where
excludeFromIndexes: trueis applied for non-array data and ensuring thatexcludeFromIndexes: trueis applied in a similar pattern for array data:Next steps:
It is clear upon inspection that one of the reasons
should pass the right properties for an array with name/value pairs and excludeLargePropertiesis failing is that the helper functionfindLargeProperties_isn't generating the rightexcludeFromIndexeslist for array values (the list is empty actually). Many test cases forfindLargeProperties_should be written andfindLargeProperties_should be modified to pass tests for the new test cases.