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

Load CDK Tests: Expand coverage of funky characters #52067

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

johnny-schmidt
Copy link
Contributor

@johnny-schmidt johnny-schmidt commented Jan 21, 2025

DO NOT MERGE UNTIL CI IS WORKING, THIS MAKES A LOT OF CHANGES TO TESTS

What

Expands coverage of testFunkyCharacters to include

  • field names with operators and leading numerals
  • stream names with special characters, operators, numbers, and leading numbers

I also found an issue in the path matching code that fails to properly escape the path matching stuff if the stream name contains legal regex. (This is also an issue with the old CDK.) I went ahead and fixed it

This breaks the recently reenabled test for glue, so I re-disabled it. @edgao I'll leave it to you to decide whether to bother renabling, and whether that means patching or adding parameters to the test.

@johnny-schmidt johnny-schmidt requested a review from a team as a code owner January 21, 2025 22:56
Copy link

vercel bot commented Jan 21, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 22, 2025 0:56am

Copy link
Contributor

@edgao edgao left a comment

Choose a reason for hiding this comment

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

had a question about the record mapper thing, plus a nit on the comment. Otherwise :shipit:

@@ -106,6 +106,7 @@ abstract class IntegrationTest(
val actualRecords: List<OutputRecord> = dataDumper.dumpRecords(config, stream)
val expectedRecords: List<OutputRecord> =
canonicalExpectedRecords.map { recordMangler.mapRecord(it, stream.schema) }
val descriptor = recordMangler.mapStreamDescriptor(stream.descriptor)
Copy link
Contributor

Choose a reason for hiding this comment

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

why have this on the record mangler, rather than requiring the data dumper to do it transparently?


@Test
@Disabled(
"Re-enabled for https://github.com/airbytehq/airbyte-internal-issues/issues/11424; TODO: fix glue"
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happily

@johnny-schmidt johnny-schmidt force-pushed the jschmidt/load-cdk/missing-test-cases-reenable-funky branch from 9abbf7b to 60d2d64 Compare January 22, 2025 00:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants