-
Notifications
You must be signed in to change notification settings - Fork 13
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
Fix all config files #614
Fix all config files #614
Conversation
internal/config/config.go
Outdated
ExpectedPass int | ||
Fail int | ||
Skip int | ||
Pass int |
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.
No, those fields should stay as is
internal/configload/stats.go
Outdated
UnexpectedFail int `yaml:"unexpected_fail"` | ||
UnexpectedSkip int `yaml:"unexpected_skip"` | ||
UnexpectedPass int `yaml:"unexpected_pass"` |
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.
We are going back and forth there. Those fields should never be present in the YAML configuration file – we don't expect the unexpected. fail
, skip
, and pass
are expected values. Unexpected values should not be in the file.
We split config
and configload
into separate packages exactly to have them separate. Why are we trying to blend them together again?
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.
My bad for that confusion you are completely right. It's a temporary fix for when we removed our support for regex, as that logic accounted for Go tests that should be skipped. So, now they will be marked as UnexpectedSkip
. All those other unexpected fields are redundant so I will remove them. I will also add a comment.
It's a little hacky but at least we have a working CI.
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.
Let's remove UnexpectedSkip
and all associated tests. Adding a comment does not change the fact that we expect unexpected things
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.
OK cool.
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 tried to remove all associated tests with unexpected skips but it didn't work as some tests rely on variables and helpers, etc. that are contained within other files.
I think I have a less tedious approach now.
I manually added the full paths to tests that should be skipped.
And
TestUnifiedSpecs
is now ignored by MongoDB too, as it seems to contain flaky tests.