Fix warnings and tests for missing script values #32382
Merged
Conversation
This file contains 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
mayya-sharipova
added
v6.5.0
>test
|
Pinging @elastic/es-core-infra |
mayya-sharipova
force-pushed
the
deprecate-script-missing-values
branch
3 times, most recently
from
336d52f to
1cd0bcb
Compare
rjernst
reviewed
Aug 10, 2018
| `-Des.scripting.exception_for_missing_value=true` on a node. If you do not enable | ||
| this behavior, a deprecation warning is logged on start up. | ||
| `-Des.scripting.exception_for_missing_value=true` on a node. If you do not | ||
| enable this behavior, a one-time deprecation warning is logged when painless |
There was a problem hiding this comment.
For other deprecated uses that potentially happen a lot, we have used deprecationLog.deprecatedAndMaybeLog. I think that would be better here. See other deprecated examples inside ScriptDocValues
For main code: - ScriptModule will NOT produce a deprecation warning anymore, when `es.scripting.exception_for_missing_value` is not set. This warning is produced only when missing values in script are used and is produced only once. For tests: - make `es.scripting.exception_for_missing_value` equal to `false` by default in tests. This behaviour is similar with default node settings, and with tests running from IDE. - modify `ScriptDocValuesMissingV6BehaviourTests` to test the default behaviour in v6, where there are default values for missing values. - add `ScriptDocValuesMissingV7BehaviourTests` to test the default behavior in v7 where `es.scripting.exception_for_missing_value` is set to `true`, and where exceptions are produced for missing values. Relates to elastic#29286
mayya-sharipova
force-pushed
the
deprecate-script-missing-values
branch
from
1cd0bcb to
2c8b9ae
Compare
|
@elasticmachine run sample packaging tests |
rjernst
reviewed
Aug 14, 2018
| `-Des.scripting.exception_for_missing_value=true` on a node. If you do not enable | ||
| this behavior, a deprecation warning is logged on start up. | ||
| `-Des.scripting.exception_for_missing_value=true` on a node. If you do not | ||
| enable this behavior, a one-time deprecation warning is logged for each |
There was a problem hiding this comment.
I think this explanation needs to be updated given the use of deprecatedAndMaybeLog?
| @@ -135,12 +128,12 @@ public Void run() { | |||
| } | |||
| }, noPermissionsAcc); | |||
|
|
|||
| assertThat(warnings, containsInAnyOrder( | |||
| assertThat(warnings, hasItems( | |||
There was a problem hiding this comment.
This seems to be making the check lenient, so other warnings could also exist. I think this test should be verify all warnings that are emitted?
| public boolean enableWarningsCheck() { | ||
| // missing values has some deprecated warnings | ||
| // that only are output once which is not easy to check | ||
| // that's why we disable warnings' checking |
There was a problem hiding this comment.
This is no longer true right? And isn't the purpose of this test to check those warnings?
mayya-sharipova
force-pushed
the
deprecate-script-missing-values
branch
from
ea36d72 to
67df5d8
Compare
|
@rjernst Ryan, can you please continue to review this PR, when you have time. I have tried to address your comments. |
rjernst
approved these changes
Sep 8, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
For main code:
when
es.scripting.exception_for_missing_valueis not set.This warning is produced only when missing values in script are used and
is produced only once.
For tests:
make
es.scripting.exception_for_missing_valueequal tofalseby default in tests. This behaviour is similar with default
node settings, and with tests running from IDE.
modify
ScriptDocValuesMissingV6BehaviourTeststo test the defaultbehaviour in v6, where there are default values for missing values.
add
ScriptDocValuesMissingV7BehaviourTeststo test the defaultbehavior in v7 where
es.scripting.exception_for_missing_valueis set to
true, and where exceptions are produced for missingvalues.
Relates to #29286