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

enable tests for app, fix bugs, disable LibraryTest.testRdd #980

Merged
merged 4 commits into from
Aug 5, 2022

Conversation

tnazarew
Copy link
Contributor

@tnazarew tnazarew commented Aug 2, 2022

Signed-off-by: tomasznazarewicz t.nazarewicz94@gmail.com

Problem

app module was only running integration tests

Original solution assumed that the test will be enabled and all potential bugs fixed. That worked for all tests except LibraryTest.testRdd, which has two problems:

  1. java.lang.NoSuchMethodError: io.openlineage.client.OpenLineageClientUtils.newObjectMapper()Lcom/fasterxml/jackson/databind/ObjectMapper;
  2. when ObjectMapper was changed there is issue with serialization, if test is run with ./gradlew app:test the tests are passing but if it's run with ./gradlew app:build the OpenLineage.RunFacets.additionalProperties is serialized as additional field and test is failing

EDIT:
It turns out there are also issues with Delta.

Closes: potentially -> https://openlineage.slack.com/archives/C01CK9T7HKR/p1658868064009839

Solution

It seems to be a blocking issue so the proposed solution for now is to disable LibraryTest.testRdd and DeltaDataSourceTest.testInsertIntoDeltaSource so everything is passing and in the meantime investigate both issues

Signed-off-by: tomasznazarewicz <t.nazarewicz94@gmail.com>
Signed-off-by: tomasznazarewicz <t.nazarewicz94@gmail.com>
Signed-off-by: tomasznazarewicz <t.nazarewicz94@gmail.com>
…ltaSource

Signed-off-by: tomasznazarewicz <t.nazarewicz94@gmail.com>
@tnazarew tnazarew requested review from mobuchowski and pawel-big-lebowski and removed request for mobuchowski August 3, 2022 09:59
@tnazarew tnazarew marked this pull request as ready for review August 3, 2022 11:02
Copy link
Contributor

@pawel-big-lebowski pawel-big-lebowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please mention new issue related to disabled tests.
Even though there are such tests, it's still better to merge it and turn on tests on app.

@tnazarew tnazarew merged commit 7fd8585 into main Aug 5, 2022
@tnazarew tnazarew deleted the spark/enable-tests-for-app branch August 5, 2022 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants