-
Notifications
You must be signed in to change notification settings - Fork 64
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
Test framework/guide #359
Test framework/guide #359
Conversation
a493867
to
89f31e1
Compare
Changes AnalysisCommit SHA: c8aaf54 API ChangesSummaryNO CHANGES ReportThe full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/9666746566/artifacts/1637069617 API Coverage
|
import { SchemaVisitor } from './utils/SpecificationVisitor' | ||
import { is_ref, type MaybeRef, SpecificationContext } from './utils' | ||
import { SchemaVisitor } from '../_utils/SpecificationVisitor' | ||
import { is_ref, type MaybeRef, SpecificationContext } from '../_utils' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for the leading underscore on _utils
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To differentiate it from other folders like tester
linter
merger
where they are tools while _utils
is not a tool.
tools/src/tester/test.ts
Outdated
} from '../OpenSearchHttpClient' | ||
import ChapterReader from './ChapterReader' | ||
import ChapterEvaluator from './ChapterEvaluator' | ||
import OperationLocator from './OperationLocator' | ||
import SchemaValidator from './SchemaValidator' | ||
import StoryEvaluator from './StoryEvaluator' | ||
import { ConsoleResultLogger } from './ResultLogger' | ||
import {ConsoleResultLogger} from './ResultLogger' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised the linter isn't complaining about this as it should be enforcing spacing around the braces (or at least it was)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I setup my IDE to run --fix
on save. I thought the linter wanted space too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks suspicious. Other files have a space. We definitely want 1 rule for spaces before/after {}
- Something is off with the eslint rule file being used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it. Didn't dig too deep but object-curly-spacing
rule was deprecated in ESLint v8.53.0 and moved to a different plugin. Installing the plugin and enabled that rule solved it.
- Reduced noise when running spec tests - `--verbose` now provides better context for non-2XX responses. - Moved `linter/utils` to `_utils` because it's also being used by the tester tool (To avoid misleading stacktrace when an error appears in utils) Signed-off-by: Theo Truong <[email protected]>
dde1822
to
0547647
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nits + let's resolve this linter problem.
Feel free to break up the PR, the doc looks good.
tools/src/tester/test.ts
Outdated
} from '../OpenSearchHttpClient' | ||
import ChapterReader from './ChapterReader' | ||
import ChapterEvaluator from './ChapterEvaluator' | ||
import OperationLocator from './OperationLocator' | ||
import SchemaValidator from './SchemaValidator' | ||
import StoryEvaluator from './StoryEvaluator' | ||
import { ConsoleResultLogger } from './ResultLogger' | ||
import {ConsoleResultLogger} from './ResultLogger' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks suspicious. Other files have a space. We definitely want 1 rule for spaces before/after {}
- Something is off with the eslint rule file being used?
Signed-off-by: Theo Truong <[email protected]>
--verbose
now provides better context for non-2XX responses.linter/utils
to_utils
because it's also being used by the tester tool (To avoid misleading stacktrace when an error appears in utils)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.