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.

Sign up for GitHub

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

Jump to bottom

Run examples in CI #220

Merged
merged 17 commits into from
Mar 23, 2022
Merged

Run examples in CI #220

merged 17 commits into from
Mar 23, 2022

Conversation

billschereriii
Copy link
Contributor

  • Update examples to provide a zero exit code on successful run
  • Add pytest script to run all examples
  • Add make target for make test-examples
  • Add run of make test examples to CI script

@billschereriii billschereriii added area: build Issues related to builds, makefiles, installs, etc area: test Issues related to the test suite area: examples Issues related to tutorials and examples labels Feb 19, 2022
@billschereriii billschereriii self-assigned this Feb 19, 2022
@codecov-commenter
Copy link

codecov-commenter commented Feb 19, 2022 •
edited
Loading

Codecov Report

Merging #220 (d16fe32) into develop (318d1b9) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #220   +/-   ##
========================================
  Coverage    82.36%   82.36%           
========================================
  Files           51       51           
  Lines         2796     2796           
========================================
  Hits          2303     2303           
  Misses         493      493

@ben-albrecht ben-albrecht self-requested a review February 22, 2022 21:21
@billschereriii
Copy link
Contributor Author

This PR is blocked by Add clustered Redis testing to SmartRedis CI.

Everything is working except for the fact that the examples are written for clustered Redis and CI uses non-clustered.

Spartee
Spartee suggested changes Mar 4, 2022
View reviewed changes
Copy link
Contributor

@Spartee Spartee left a comment

Choose a reason for hiding this comment

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

These should be in a different testing file (yml file) from run_test.yml. Given that SR already has a whopping 34 checks for each check-in, lets run these once ever merged PR.

Changes:

  • Separate workflow file
  • run once every PR to develop.

ben-albrecht
ben-albrecht reviewed Mar 23, 2022
View reviewed changes
Copy link
Contributor

@ben-albrecht ben-albrecht left a comment

Choose a reason for hiding this comment

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

CI and Makefile changes LGTM, barring one inline comment

ben-albrecht
ben-albrecht approved these changes Mar 23, 2022
View reviewed changes
Copy link
Contributor

@ben-albrecht ben-albrecht left a comment

Choose a reason for hiding this comment

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

Rest of the changes LGTM.

examples/test_examples.py
print(f"Test command {' '.join(cmd)}")
print(f"Using cluster: {use_cluster}")
execute_cmd(cmd)
time.sleep(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this sleep?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, I'm not sure. It was in the other test driver scripts so I kept it there

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to understand the rationale and document it as a comment here for our future selves, but that's a nit (not PR-blocking).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Spartee Do you happen to know?

Copy link
Contributor

Choose a reason for hiding this comment

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

on shared systems sometimes it can take an extra couple seconds to spin up

Copy link
Contributor

Choose a reason for hiding this comment

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

but we've optimized this since this was needed so my guess would be that it's not anymore

Spartee
Spartee approved these changes Mar 23, 2022
View reviewed changes
Copy link
Contributor

@Spartee Spartee left a comment

Choose a reason for hiding this comment

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

Thanks for those changes. this looks good.

@billschereriii billschereriii merged commit 0767ba7 into CrayLabs:develop Mar 23, 2022
@billschereriii billschereriii deleted the ci-examples branch March 23, 2022 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ben-albrecht ben-albrecht ben-albrecht approved these changes

@Spartee Spartee Spartee approved these changes

@mellis13 mellis13 Awaiting requested review from mellis13

@EricGustin EricGustin Awaiting requested review from EricGustin

Assignees

@billschereriii billschereriii

Labels
area: build Issues related to builds, makefiles, installs, etc area: examples Issues related to tutorials and examples area: test Issues related to the test suite
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@billschereriii @codecov-commenter @ben-albrecht @Spartee