Skip to content
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

assertIngestDocument() is weaker than it appears #28492

Closed
DaveCTurner opened this issue Feb 2, 2018 · 2 comments
Closed

assertIngestDocument() is weaker than it appears #28492

DaveCTurner opened this issue Feb 2, 2018 · 2 comments
Assignees
Labels
:Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >test Issues or PRs that are addressing/adding tests

Comments

@DaveCTurner
Copy link
Contributor

It seems that IngestDocumentMatcher#assertIngestDocument() does not look at the content of its arguments too carefully. The following test does not pass:

diff --git a/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java b/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java
index 9df2a38c6f..0ab5d79457 100644
--- a/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java
+++ b/server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java
@@ -1021,4 +1021,13 @@ public class IngestDocumentTests extends ESTestCase {
         }
     }
 
+    public void testEqualityAssertion() {
+        Map<String, Object> sourceAndMetadata1 = new HashMap<>();
+        sourceAndMetadata1.put("foo", "bar");
+        IngestDocument document1 = new IngestDocument(sourceAndMetadata1, new HashMap<>());
+
+        IngestDocument document2 = new IngestDocument(new HashMap<>(), new HashMap<>());
+
+        expectThrows(AssertionError.class, () -> assertIngestDocument(document1, document2));
+    }
 }

Found during the course of #28476

@DaveCTurner DaveCTurner added >test Issues or PRs that are addressing/adding tests :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP labels Feb 2, 2018
@talevy
Copy link
Contributor

talevy commented Feb 20, 2018

missed this. thanks for the ping... I don't quite remember why this assertion behaves as it does anymore :(. I remember I was more interested in the general shape of the document, and I understand this fails that as well.

looking more closely... this fails almost an infinite of other cases as well... which leads me to mis-remember why it was written how it was in the first place 😄. I'll take a look, thanks @DaveCTurner

@original-brownbear
Copy link
Member

Fixed in #31913 I think :)

@talevy talevy removed the help wanted adoptme label Jul 10, 2018
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this issue Jul 10, 2018
* Previously docA being subset of docB passed because iteration was over docA's keys only
* Scalars in nested fields were not compared in all cases
* Assertion errors were hard to interpret (message wasn't correct since it only mentioned the class type)
* In cases where two paths contained different types a ClassCastException was thrown instead of an AssertionError
* Fixes elastic#28492
original-brownbear added a commit that referenced this issue Jul 11, 2018
* Fix assertIngestDocument wrongfully passing

* Previously docA being subset of docB passed because iteration was over docA's keys only
* Scalars in nested fields were not compared in all cases
* Assertion errors were hard to interpret (message wasn't correct since it only mentioned the class type)
* In cases where two paths contained different types a ClassCastException was thrown instead of an AssertionError
* Fixes #28492
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this issue Jul 11, 2018
* Fix assertIngestDocument wrongfully passing

* Previously docA being subset of docB passed because iteration was over docA's keys only
* Scalars in nested fields were not compared in all cases
* Assertion errors were hard to interpret (message wasn't correct since it only mentioned the class type)
* In cases where two paths contained different types a ClassCastException was thrown instead of an AssertionError
* Fixes elastic#28492
original-brownbear added a commit that referenced this issue Jul 12, 2018
* Fix assertIngestDocument wrongfully passing

* Previously docA being subset of docB passed because iteration was over docA's keys only
* Scalars in nested fields were not compared in all cases
* Assertion errors were hard to interpret (message wasn't correct since it only mentioned the class type)
* In cases where two paths contained different types a ClassCastException was thrown instead of an AssertionError
* Fixes #28492
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >test Issues or PRs that are addressing/adding tests
Projects
None yet
Development

No branches or pull requests

4 participants