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

Refactor test workflow #4457

Merged
merged 11 commits into from
May 11, 2023
Merged

Refactor test workflow #4457

merged 11 commits into from
May 11, 2023

Conversation

eyurtsev
Copy link
Collaborator

@eyurtsev eyurtsev commented May 10, 2023

Refactor the test workflow

This PR refactors the tests to run using a single test workflow. This makes it easier to relaunch failing tests and see in the UI which test failed since the jobs are grouped together.

Before submitting

Who can review?

@eyurtsev eyurtsev marked this pull request as ready for review May 10, 2023 18:49
@dev2049
Copy link
Contributor

dev2049 commented May 10, 2023

will they still be run in parallel?

@eyurtsev
Copy link
Collaborator Author

@dev2049 yeah all envs run in parallel when using a matrix strategy

run: |
make test
make tests
shell: bash
Copy link
Contributor

Choose a reason for hiding this comment

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

why is shell: bash needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rule of thumb is to always specify the shell in all workflows. The shell turns on strict mode which can make it much faster to debug.

run: |
make test
make tests
Copy link
Contributor

Choose a reason for hiding this comment

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

why the rename, and why do we have both?

Copy link
Contributor

Choose a reason for hiding this comment

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

as in why rename here, and why do we have both in makefile

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh rename is totally accidental, but it does the same thing. I'll follow up with a minor typo fix in a second after landing this.

Copy link
Contributor

@dev2049 dev2049 left a comment

Choose a reason for hiding this comment

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

one qq, otherwise lgtm!

@eyurtsev eyurtsev merged commit f373883 into master May 11, 2023
@eyurtsev eyurtsev deleted the eugene/update_tests branch May 11, 2023 01:57
jpzhangvincent pushed a commit to jpzhangvincent/langchain that referenced this pull request May 12, 2023
# Refactor the test workflow

This PR refactors the tests to run using a single test workflow. This
makes it easier to relaunch failing tests and see in the UI which test
failed since the jobs are grouped together.

## Before submitting

## Who can review?
EandrewJones pushed a commit to Oogway-Technologies/langchain that referenced this pull request May 12, 2023
# Refactor the test workflow

This PR refactors the tests to run using a single test workflow. This
makes it easier to relaunch failing tests and see in the UI which test
failed since the jobs are grouped together.

## Before submitting

## Who can review?
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