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

Preview Build Failure: Test Case Refactoring proposal [Work in Progress] #3043

Closed
wants to merge 11 commits into from

Conversation

sbera87
Copy link
Contributor

@sbera87 sbera87 commented Jul 16, 2024

Issue #, if available:

Description of changes:

Check all that applies:

  • Did a review by yourself.
  • Added proper tests to cover this PR. (If tests are not applicable, explain.)
  • Checked if this PR is a breaking (APIs have been changed) change.
  • Checked if this PR will not introduce cross-platform inconsistent behavior.
  • Checked if this PR would require a ReadMe/Wiki update.

Check which platforms you have built SDK on to verify the correctness of this PR.

  • Linux
  • Windows
  • Android
  • MacOS
  • IOS
  • Other Platforms

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@@ -77,8 +77,8 @@ Aws::String BuildTableName(const char* baseName)
class TableOperationTest : public ::testing::Test {

public:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These changes allows tests to be run individually using gtest_filter enabling developers to debug case by case, as opposed to having to run all the tests for just even one test to run successfully.

Change is moving from static setup/cleanup to fixture level setup/cleanup in line with how S3 tests have it


*/

TEST_F(TableOperationTest, TestCrudOperations2)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Demonstration of Proposal for refactoring existing test case to be more robust and resilient .
Check out the forced failure to demonstrate retries for one functional block

res = res && (hashKeyName.str() == returnedItemCollection[HASH_KEY_NAME].GetS());
res = res && (testValueColumn.str() == returnedItemCollection[testValueColumnName].GetS());

if(numRetriesLeft > 0)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forced failure to try multiple times till exhaustion leading to success and subsequent GTEST MACRO evaluation

{
AWS_LOGSTREAM_TRACE(ALLOCATION_TAG, "TestCrudOperations")

struct TestContext{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test specific context. User defined


deleteRequestIds = requestIds;

auto putItemReqFunction = [testValueColumnName, crudTestTableName, this, &requestIds](
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example of a function block. Capture whatever is needed and adheres to the function signature of the test planner class

}
else
{
it = requestIds.erase(it);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: In this test specific setup, on retries of the function block it only retries elements that had failed.
It is accomplished by maintaining a list which is random node delete friendly.

If a get item request is processed successfully, it's removed .

}
AWS_LOGSTREAM_TRACE(ALLOCATION_TAG, "added function with id=" << item.entryId<<" num tries="<<item.retryCount<<" stop on fail="<<item.stopOnFail<<std::endl );
//std::cout<<"added function with id=" << item.entryId<<std::endl;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

print to std outs will be removed before final draft

}
private:
using FunctionSuite = std::list<FunctionBlock>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can use any container, I used generic iterator interface for this reason. Can make this a template parameter with type traits checks. This is just enhancement

@@ -100,6 +100,8 @@ class TableOperationTest : public ::testing::Test {
std::mutex updateItemResultMutex;
std::condition_variable updateItemResultSemaphore;

std::vector<Aws::String> m_tablesCreated;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tracks tables created, which gets cleaned up in the fixture destructor invocation

Aws::UnorderedSet<Aws::String> _uniqueFunctionBlockSet;
size_t _cursor;

static const char ALLOCATION_TAG[];

//compile time conditional invoking of callable function with different arguments depending upon available context or not in c++ 11

using RetryFunctionWrapped = std::function<bool (const RetryFunction_t& callable, const BlockIdentifierType& , size_t) >;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a way for the Retry Planner class to not mandate context where test case doesn't need one. This is implemented using SFINAE concepts and type traits


//set global status from block evaluation
status = status && localResult;

//if stop on fail after retries, break and retry from start if available
if(it->stopOnFail && !localResult)
if(( it->stopStrategy == StopStrategy::STOP_ON_FIRST_FAIL || it->stopStrategy == StopStrategy::STOP_AFTER_ALL_RETRIES ) && !localResult)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a retry plan stop strategy as needed by use case.
One can choose to continue even on failure, stop after first failures or stop after exhausting all retries.

@sbera87
Copy link
Contributor Author

sbera87 commented Aug 1, 2024

After delibration, we decided on step level retries

@sbera87 sbera87 closed this Aug 1, 2024
@sbera87 sbera87 deleted the pbver branch November 25, 2024 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant