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

SmartRedis test system refactor #356

Merged
merged 52 commits into from
Jun 29, 2023
Merged

Conversation

billschereriii
Copy link
Contributor

Massive overhaul of test system, automating launch and teardown of Redis and removing the need for the setup_test_env.sh script

@billschereriii billschereriii self-assigned this Jun 22, 2023
@codecov
Copy link

codecov bot commented Jun 27, 2023

Codecov Report

Merging #356 (efa0b68) into develop (11248bb) will increase coverage by 78.36%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##           develop     #356       +/-   ##
============================================
+ Coverage         0   78.36%   +78.36%     
============================================
  Files            0       97       +97     
  Lines            0     7057     +7057     
============================================
+ Hits             0     5530     +5530     
- Misses           0     1527     +1527     

see 97 files with indirect coverage changes

Copy link
Member

@MattToast MattToast left a comment

Choose a reason for hiding this comment

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

Looks amazing!

I went on a deep dive into the launch_redis.py file basically just trying to remove the need for shell=True in the Popen calls and calling out some style things along the way.

I skimmed through the Makefile as well and that, to the best of my ability looks great with one outstanding questiong.

Overall, only some pedantic feedback! LGTM!!

@billschereriii billschereriii requested a review from MattToast June 29, 2023 20:13
Copy link
Collaborator

@ashao ashao left a comment

Choose a reason for hiding this comment

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

Looks good to me, with the exception of the one change to the detection of redis-cli. This should avoid the need to set another environment variable.

Thanks for this; it's a HUGE improvement

is_uds = udsport is not None
if is_uds:
n_nodes = 1
cicd = os.getenv('SR_CICD_EXECUTION')
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the easier way to do this is to use shutil.which('redis-cli') and then set the rediscli variable at the top of the file and/or make it into a small function

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 actually have fairly substantial changes based on whether we're in CI/CD or not, so this isn't the only thing affected. Can discuss more offline

Copy link
Member

@MattToast MattToast left a comment

Choose a reason for hiding this comment

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

LGTM!! Ship it!!

@billschereriii billschereriii merged commit 4e72023 into CrayLabs:develop Jun 29, 2023
@billschereriii billschereriii deleted the srtest branch June 29, 2023 22:51
ashao pushed a commit to ashao/SmartRedis that referenced this pull request Jul 6, 2023
Revamp SmartRedis test environment
[ committed by @billschereriii ]
[ reviewed by @ashao @MattToast ]
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