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

GCS cloudbuild e2e fixes #1462

Merged

Conversation

AnkitCLI
Copy link
Contributor

@AnkitCLI AnkitCLI commented Nov 26, 2024

This PR contains fix for the error message mismatch for GCS on cloudbuild

@AnkitCLI AnkitCLI added the build Trigger unit test build label Nov 26, 2024
@AnkitCLI AnkitCLI force-pushed the GCS-cloudbuild-fixes branch from bc9242b to 844e322 Compare November 26, 2024 18:36
Copy link
Member

@itsankit-google itsankit-google left a comment

Choose a reason for hiding this comment

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

why do these tests not have Open and capture logs step like other tests?

Can we ensure these tests are consistent. This will also help us in verifying the pipeline logs and assumption by e2e tests.

image

@AnkitCLI AnkitCLI force-pushed the GCS-cloudbuild-fixes branch from 844e322 to 527e4e6 Compare November 27, 2024 06:26
@AnkitCLI
Copy link
Contributor Author

why do these tests not have Open and capture logs step like other tests?

Can we ensure these tests are consistent. This will also help us in verifying the pipeline logs and assumption by e2e tests.

image

added the step for both scenarios.

Then Open Pipeline logs and verify Log entries having below listed Level and Message:
| Level | Message |
| ERROR | errorMessageMultipleFileWithFirstRowAsHeaderEnabled |
| WARN | errorMessageMultipleFileWithFirstRowAsHeaderEnabled |
Copy link
Member

Choose a reason for hiding this comment

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

how is this WARN?

I can see the level is ERROR:
image

How is this test passing right now?

Copy link
Contributor Author

@AnkitCLI AnkitCLI Nov 27, 2024

Choose a reason for hiding this comment

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

Basically to make it compatible with both cdap and CDF we use warn here because in cdap it only show NumberFormatException error in only WARN level in advanced logs. You are checking it in raw logs. But the above mentioned step only checks in advanced logs section.

Copy link
Member

Choose a reason for hiding this comment

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

Can we modify the step to check in raw logs? So that it is consistent in both cdap and CDF.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to the common errorMessage for both cdap and CDF in ERROR level.

@AnkitCLI AnkitCLI force-pushed the GCS-cloudbuild-fixes branch from 527e4e6 to 697b995 Compare November 27, 2024 16:45
@AnkitCLI AnkitCLI force-pushed the GCS-cloudbuild-fixes branch from 697b995 to 52dbbfe Compare November 28, 2024 05:40
@itsankit-google itsankit-google merged commit 692dfcd into data-integrations:develop Nov 28, 2024
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Trigger unit test build
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants