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

Re-run persistency tests on AOF-enabled servers #1896

Merged
merged 3 commits into from
Nov 24, 2021
Merged

Re-run persistency tests on AOF-enabled servers #1896

merged 3 commits into from
Nov 24, 2021

Conversation

jeffreylovitz
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Aug 25, 2021

Codecov Report

Merging #1896 (435156b) into master (1951427) will not change coverage.
The diff coverage is n/a.

❗ Current head 435156b differs from pull request most recent head 10d4785. Consider uploading reports for the commit 10d4785 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1896   +/-   ##
=======================================
  Coverage   92.76%   92.76%           
=======================================
  Files         247      247           
  Lines       21648    21648           
=======================================
  Hits        20081    20081           
  Misses       1567     1567           
Impacted Files Coverage Δ
src/util/thpool/thpool.c 81.13% <0.00%> (-1.26%) ⬇️
...rc/serializers/encoder/v10/encode_graph_entities.c 97.18% <0.00%> (+1.40%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1951427...10d4785. Read the comment docs.

else
# mac
ifeq ($(VALGRIND),)
# no valgrind
@python3 -m RLTest --module ../../src/redisgraph.so $(TEST_ARGS)
# rerun persistency tests using AOF
@python3 -m RLTest --module ../../src/redisgraph.so --use-aof -t test_persistency $(TEST_ARGS)
Copy link
Contributor

Choose a reason for hiding this comment

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

@MeirShpilraien
Can --use-aof be specified by the actual test file? as an argument to RLTest.Env ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Env accepts a useAof argument, but two environments can't be live simultaneously, so we would need to duplicate the tests to run under both RDB and AOF.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jeffreylovitz @MeirShpilraien
how about we introduce 2 new classes both inheriting from testGraphPersistency?

class testGraphPersistency(FlowTestsBase):
    def __init__(self, useAof):
        self.env = Env(decodeResponses=True, useAof=useAof)
        ...

class testGraphPersistencyAOF(testGraphPersistency):
    super(testGraphPersistency, True)
  
class testGraphPersistencyRDB(testGraphPersistency):
    super(testGraphPersistency, False)

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can as well define a shared function that both tests calls to after creating the env, but I remember @jeffreylovitz wanted to avoid this. This is why I suggested this way..

@swilly22 swilly22 merged commit 53ddab0 into master Nov 24, 2021
@swilly22 swilly22 deleted the test-aof branch November 24, 2021 07:01
jeffreylovitz added a commit that referenced this pull request Dec 7, 2021
* Re-run persistency tests on AOF-enabled servers

* clean-up

Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>
Co-authored-by: swilly22 <roi@redislabs.com>
(cherry picked from commit 53ddab0)
pnxguide pushed a commit to CMU-SPEED/RedisGraph that referenced this pull request Mar 22, 2023
* Re-run persistency tests on AOF-enabled servers

* clean-up

Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>
Co-authored-by: swilly22 <roi@redislabs.com>
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.

3 participants