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

Added Tests for the test framework #310

Merged
merged 1 commit into from
Jun 4, 2024

Conversation

nhtruong
Copy link
Collaborator

@nhtruong nhtruong commented Jun 4, 2024

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link
Contributor

github-actions bot commented Jun 4, 2024

Changes Analysis

Commit SHA: 59c2064
Comparing To SHA: 6bdfbc3

API Changes

Summary

NO CHANGES

Report

The full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/9372884295/artifacts/1568212464

API Coverage

Before After Δ
Covered (%) 476 (46.62 %) 476 (46.62 %) 0 (0 %)
Uncovered (%) 545 (53.38 %) 545 (53.38 %) 0 (0 %)
Unknown 24 24 0

dblock
dblock previously approved these changes Jun 4, 2024
Copy link
Member

@dblock dblock 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. Fix CI, + some minor things.

I do recommend putting back the empty.yaml as a a minimal test of passing something that has no actual tests in it.

DEVELOPER_GUIDE.md Outdated Show resolved Hide resolved
DEVELOPER_GUIDE.md Outdated Show resolved Hide resolved
Comment on lines 13 to 14
// The password must match the one specified in .github/workflows/test-spec.yml
process.env.OPENSEARCH_PASSWORD = 'myStrongPassword123!'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we hard coding a password here, this should just be set in the workflow

Comment on lines 7 to 8
// The password must match the one specified in .github/workflows/test-spec.yml
process.env.OPENSEARCH_PASSWORD = 'myStrongPassword123!'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we hard coding a password here, this should just be set in the workflow

Copy link
Collaborator Author

@nhtruong nhtruong Jun 4, 2024

Choose a reason for hiding this comment

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

I should change it to

process.env.OPENSEARCH_PASSWORD =  process.env.OPENSEARCH_PASSWORD ?? 'myStrongPassword123!'`

The idea is streamlining local testing for the user. When they run docker compose up, this is the same password the cluster will be setup with by default. So, they can quickly run the tests right after the contain is set up and ready.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not a security concern since this is just for testing and the password is already shown in the workflow.

@nhtruong
Copy link
Collaborator Author

nhtruong commented Jun 4, 2024

Figuring out why the tests fail here but not locally. Changing this to draft. Please ignore the commit pushes till I change it back to reviewable.

@nhtruong nhtruong marked this pull request as draft June 4, 2024 17:07
@nhtruong
Copy link
Collaborator Author

nhtruong commented Jun 4, 2024

@dblock We currently does not allow an empty story because the chapters array must have at least one element in it. The deleted empty.yaml story is, therefore, not a valid story. I'll add a schema validator for each story after this PR.

Signed-off-by: Theo Truong <theotr@amazon.com>
@nhtruong nhtruong marked this pull request as ready for review June 4, 2024 19:17
@nhtruong
Copy link
Collaborator Author

nhtruong commented Jun 4, 2024

It's ready for review now

@Xtansia Xtansia merged commit 7a0d9f5 into opensearch-project:main Jun 4, 2024
7 checks passed
@nhtruong nhtruong deleted the test_framework/tests branch June 5, 2024 02:14
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