Small improvements related to types deprecation. by jtibshirani · Pull Request #36328 · elastic/elasticsearch

jtibshirani · 2018-12-06T18:51:48Z

Make sure to use deprecatedAndMaybeLog for types deprecation messages.
Introduce a common base class for Rest*Action tests.

elasticmachine · 2018-12-06T18:51:51Z

Pinging @elastic/es-core-infra

nik9000

Nice! I left a small comment. I wonder if these is some way to clean up RestGetSourceActionTests. Could we make a method that makes the listener rather than set it up every time? That feels a little cleaner but I'm not sure.

nik9000 · 2018-12-06T19:21:57Z

server/src/test/java/org/elasticsearch/rest/action/RestActionTestCase.java

+
+import static org.mockito.Mockito.mock;
+
+public abstract class RestActionTestCase extends ESTestCase {


This is great! Please add javadoc!

nik9000 · 2018-12-06T19:22:39Z

server/src/test/java/org/elasticsearch/rest/action/RestActionTestCase.java

+
+    public void setUp() throws Exception {
+        super.setUp();
+        controller = new RestController(Collections.emptySet(), null,


I think it is worth a comment about why these aren't final.

Are you imagining a comment like 'these are effectively final, but can't be marked as such because they're initialized in the @Before method`?

Could you declare the controller as final? I think it looks like it'd work.

I caught up with @nik9000 offline and he is okay sticking with the style where we use @Before methods (and instance variables aren't final), instead of switching everything over to instance variables/ constructors. The reasoning is that there is an existing convention to use @Before methods in JUnit tests as opposed to standard object construction.

I tend to use @Before as little as possible because it looks to me like a signal that I'm doing something complex. I see that this might be out of step with normal maven usage though. So I'm totally fine not doing it.

nik9000 · 2018-12-06T19:22:55Z

server/src/test/java/org/elasticsearch/rest/action/RestActionTestCase.java

+public abstract class RestActionTestCase extends ESTestCase {
+    private RestController controller;
+
+    public void setUp() throws Exception {


I think it is weird to override the superclass setup rather than make your own.

Sorry, I'm having trouble understanding the right alternative. This looked like a common approach in abstract classes that extend ESTestCase.

It comes up some times but other times we make a new @Before method instead. I think most of the time we make a @Before method. Unless we have specific ordering requirements between the method and the one of the superclass.

jtibshirani · 2018-12-06T20:24:05Z

Thanks @nik9000 for the speedy code review! For RestGetSourceActionTests, could you explain a little more about what you were thinking? I didn't see a good way to pull more common functionality into RestActionTestCase, since the test is doing something fairly specific to 'get source' actions.

mayya-sharipova

Thanks Julie, your PR makes the code much shorter

mayya-sharipova · 2018-12-06T20:25:20Z

server/src/test/java/org/elasticsearch/rest/action/RestActionTestCase.java

+
+public abstract class RestActionTestCase extends ESTestCase {
+    private RestController controller;
+


should we add @Override above?

Now that we're creating new @Before methods, we should be able to avoid overriding setUp.

nik9000 · 2018-12-06T20:58:52Z

I didn't see a good way to pull more common functionality into RestActionTestCase, since the test is doing something fairly specific to 'get source' actions.

I don't think you should pull more common functionality into RestActionTestCase. I was more wondering if we needed the change there at all. Or, if we need it, could we continue to make them final and declared at the top? I think it is easier to understand what is going on when we avoid @Before methods that we don't absolutely need. Mostly because when I see a before method that doesn't look like we need it my brain flies off on a tangent trying to figure out why we have the method.

jtibshirani · 2018-12-06T21:31:38Z

I was more wondering if we needed the change there at all.

Got it! I was used to a JUnit style where @Before is simply used in place of a constructor. So I thought this refactor was cleaning up RestGetSourceActionTests, but you actually found it confusing. I'll just revert this part of the PR, as it's not necessary.

mayya-sharipova · 2018-12-06T21:36:37Z

...-mustache/src/main/java/org/elasticsearch/script/mustache/RestMultiSearchTemplateAction.java

                    if (searchRequest.types().length > 0) {
-                        deprecationLogger.deprecated(RestMultiSearchAction.TYPES_DEPRECATION_MESSAGE);
+                        deprecationLogger.deprecatedAndMaybeLog("msearch_template_with_types",
+                            RestMultiSearchAction.TYPES_DEPRECATION_MESSAGE);


Do we want to use RestMultiSearchTemplateAction here and the same with logger on line 44?

Thanks for pointing this out @mayya-sharipova. If it's okay with you, I'll plan to fix this in another PR as this one is already reasonably large.

An update: I've fixed this in #36344.

…ork.

jtibshirani · 2018-12-06T23:54:58Z

After finding several additional instances of Rest*ActionTests that are outside of this package, I decided it would be good to scope this PR to just the test classes we've added to test types deprecation. That way I won't get pulled into a huge refactor, and we can make sure to have this base class available to others as they work on additional deprecations. @nik9000 @mayya-sharipova does this sound reasonable to you?

Here are the items I plan to follow up on in subsequent PRs:

Make sure RestMultiSearchTemplateAction uses the right class for logging and deprecation messages.
Refactor RestValidateQueryActionTests to simplify test set-up, and be able to use the base class RestActionTestCase.
Look into cleaning up other Rest*ActionTests by pulling them into the right package, unifying test set-up, etc.

nik9000 · 2018-12-07T15:41:49Z

scope this PR to just the test classes we've added to test types deprecation

Sounds good to me!

jtibshirani · 2018-12-07T18:47:05Z

Thanks, in that case I think this is ready for another look!

nik9000

LGTM

nik9000 · 2018-12-07T19:18:00Z

server/src/test/java/org/elasticsearch/rest/action/RestActionTestCase.java

+
+    public void setUp() throws Exception {
+        super.setUp();
+        controller = new RestController(Collections.emptySet(), null,


I tend to use @Before as little as possible because it looks to me like a signal that I'm doing something complex. I see that this might be out of step with normal maven usage though. So I'm totally fine not doing it.

jtibshirani added 2 commits December 6, 2018 10:24

Make sure to use deprecatedAndMaybeLog for types deprecation messages.

799b026

Introduce a common base class for Rest*Action tests.

d71a8ed

jtibshirani added >test Issues or PRs that are addressing/adding tests :Core/Infra/REST API REST infrastructure and utilities v7.0.0 labels Dec 6, 2018

jtibshirani requested review from markharwood and mayya-sharipova December 6, 2018 18:51

nik9000 reviewed Dec 6, 2018

View reviewed changes

mayya-sharipova approved these changes Dec 6, 2018

View reviewed changes

mayya-sharipova reviewed Dec 6, 2018

View reviewed changes

jtibshirani added 4 commits December 6, 2018 15:37

Add javadoc to RestActionTestCase.

930a283

Introduce specific @before methods instead of overriding setUp.

bab071b

Undo the unnecessary changes to RestGetSourceActionTests.

5fd9dea

Remove changes to tests that are unrelated to the types deprecation w…

9ecf3b8

…ork.

nik9000 approved these changes Dec 7, 2018

View reviewed changes

jtibshirani merged commit 51e1d40 into elastic:master Dec 7, 2018

jtibshirani deleted the refactor-types-deprecation branch December 7, 2018 19:21

This was referenced Dec 7, 2018

Deprecate types in update_by_query and delete_by_query #36365

Merged

For msearch templates, make sure to use the right name for deprecation logging. #36344

Merged

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019


		import static org.mockito.Mockito.mock;

		public abstract class RestActionTestCase extends ESTestCase {


		public abstract class RestActionTestCase extends ESTestCase {
		private RestController controller;

Conversation

jtibshirani commented Dec 6, 2018

Uh oh!

elasticmachine commented Dec 6, 2018

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtibshirani commented Dec 6, 2018

Uh oh!

mayya-sharipova left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Dec 6, 2018

Uh oh!

jtibshirani commented Dec 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtibshirani commented Dec 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nik9000 commented Dec 7, 2018

Uh oh!

jtibshirani commented Dec 7, 2018

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jtibshirani commented Dec 6, 2018 •

edited

Loading