-
Notifications
You must be signed in to change notification settings - Fork 124
Add ability to skip tests #221
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
Merged
Merged
Changes from all commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
79e2647
Adding skipped to reporters
ycombinator 255cdbc
Adding skipped to pipeline test runner
ycombinator 514e05e
Adding skipped to system test runner
ycombinator be3c26c
Adding skipped field to test result
ycombinator 35aa187
Add skip configuration for asset loading tests
ycombinator 078963c
Refactoring: extracting result composer for reuse in multiple packages
ycombinator 71e99e3
Use result composer in asset loading test runner
ycombinator 706263f
Adding godoc
ycombinator a994a4d
Refactoring: extract skip config struct
ycombinator a9d89f8
Adding license header + struct godoc
ycombinator 1513b04
Account for no test config
ycombinator fb6b90e
Fix error message
ycombinator 9feae09
Adding skip section to test package
ycombinator 5ba06fb
Add warning logs for skipped tests
ycombinator c409d93
Include test result
ycombinator feadfbe
Stringify link
ycombinator 3998403
Using result
ycombinator 1c9dc9a
Fixing format
ycombinator 91b97ed
Make skipped optional
ycombinator 9e66712
Add skip reason and link
ycombinator 4e25215
Introducing SkippableConfig
ycombinator d93be29
Introducing constructor for ResultComposer
ycombinator 322c835
Temporarily commenting out skip so test runs again
ycombinator cec86cf
Making test fail with bad path
ycombinator 0fbf9ad
Putting back skip section
ycombinator b195c06
Merge branch 'master' into test-skip
ycombinator File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
// or more contributor license agreements. Licensed under the Elastic License; | ||
// you may not use this file except in compliance with the Elastic License. | ||
|
||
package asset | ||
|
||
import ( | ||
"io/ioutil" | ||
"os" | ||
"path/filepath" | ||
|
||
"github.com/elastic/go-ucfg" | ||
"github.com/elastic/go-ucfg/yaml" | ||
"github.com/pkg/errors" | ||
|
||
"github.com/elastic/elastic-package/internal/testrunner" | ||
) | ||
|
||
type testConfig struct { | ||
testrunner.SkippableConfig `config:",inline"` | ||
} | ||
|
||
func newConfig(assetTestFolderPath string) (*testConfig, error) { | ||
configFilePath := filepath.Join(assetTestFolderPath, "config.yml") | ||
|
||
// Test configuration file is optional for asset loading tests. If it | ||
// doesn't exist, we can return early. | ||
if _, err := os.Stat(configFilePath); os.IsNotExist(err) { | ||
return nil, nil | ||
} | ||
|
||
data, err := ioutil.ReadFile(configFilePath) | ||
if err != nil { | ||
return nil, errors.Wrapf(err, "could not load asset loading test configuration file: %s", configFilePath) | ||
} | ||
|
||
var c testConfig | ||
cfg, err := yaml.NewConfig(data, ucfg.PathSep(".")) | ||
if err != nil { | ||
return nil, errors.Wrapf(err, "unable to load asset loading test configuration file: %s", configFilePath) | ||
} | ||
if err := cfg.Unpack(&c); err != nil { | ||
return nil, errors.Wrapf(err, "unable to unpack asset loading test configuration file: %s", configFilePath) | ||
} | ||
|
||
return &c, nil | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 think I have a general concern, but I'm sure if it can be solved. I'm worried that one day we'll add a new test runner, in which we'll forget to add the "Skip" config section. Maybe we should limit this to minimum. Do we need a skip check for assets tests (same for install)?
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 is a good point. Maybe this goes well with your other suggestion of a
commonTestConfig
. Let me try something and update this PR.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.
In 4e25215, I introduced a
SkippableConfig
struct that could be embedded in any test config structs that allow skipping. This doesn't necessarily address the concern you are bringing up (a future test runner forgetting to include the skip config section) but I think it can help make it a bit easier to add a skip config section to a future test runner. I think to truly address your concern a larger refactor might be needed which would allow a higher-level generic test runner of some sort to handle skipping before delegating the actual running of a test to specific types of test runners.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.
... and we may think about such refactoring exercise soon, as we have a better clarity of what do we expect from test runners and what kind of tests we need. Definitely we can postpone it at the moment.
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.
Agreed. Added #238 to track the refactoring.