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

Added Tests for the test framework #310

Merged
merged 1 commit into from
Jun 4, 2024
Merged

Added Tests for the test framework #310

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.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 4, 2024 •
edited
Loading

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

@nhtruong nhtruong mentioned this pull request Jun 4, 2024
Merged
dblock
dblock previously approved these changes Jun 4, 2024
View reviewed changes
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
Xtansia
Xtansia requested changes Jun 4, 2024
View reviewed changes
tools/tests/tester/helpers.ts Outdated
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

tools/tests/tester/TestsRunner.test.ts Outdated
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 •
edited
Loading

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 •
edited
Loading

@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.

@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
Reviewers

@Xtansia Xtansia Xtansia approved these changes

@dblock dblock dblock left review comments

@harshavamsi harshavamsi Awaiting requested review from harshavamsi harshavamsi is a code owner

@sachetalva sachetalva Awaiting requested review from sachetalva sachetalva is a code owner

@VachaShah VachaShah Awaiting requested review from VachaShah VachaShah is a code owner

@Tokesh Tokesh Awaiting requested review from Tokesh Tokesh is a code owner

@aabeshov aabeshov Awaiting requested review from aabeshov aabeshov is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nhtruong @dblock @Xtansia