Skip to content

test(generator): make failures more readable#7019

Merged
coryan merged 2 commits intogoogleapis:mainfrom
coryan:test-make-generator-test-more-readable
Jul 19, 2021
Merged

test(generator): make failures more readable#7019
coryan merged 2 commits intogoogleapis:mainfrom
coryan:test-make-generator-test-more-readable

Conversation

@coryan
Copy link
Copy Markdown
Contributor

@coryan coryan commented Jul 19, 2021 •
edited
Loading

Using a TestWithParam<absl::string_view> produces very unreadable
failures: the string view is printed one character at a time. And we
should use ASSSERT_STATUS_OK() before using an StatusOr<T>.

This change is Reviewable

@coryan
test(generator): make failures more readable
94a5958 
Using a `TestWithParam<absl::string_view>` produces very unreadable
failures: the string view is printed one character at a time. And we
should use `ASSSERT_STATUS_OK()` before using an `StatusOr<T>`.
@google-cla google-cla Bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 19, 2021
@google-cloud-cpp-bot
Copy link
Copy Markdown
Contributor

google-cloud-cpp-bot commented Jul 19, 2021

Google Cloud Build Logs
For commit: 94a595864c3287ff52e768c56a51cc659ff6f526

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 19, 2021 •
edited
Loading

Codecov Report

Merging #7019 (2215b84) into main (8ca03c6) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7019      +/-   ##
==========================================
- Coverage   94.54%   94.54%   -0.01%     
==========================================
  Files        1305     1305              
  Lines      114705   114704       -1     
==========================================
- Hits       108449   108445       -4     
- Misses       6256     6259       +3
Impacted Files Coverage Δ
...or/integration_tests/generator_integration_test.cc 97.18% <100.00%> (-0.04%) ⬇️
...le/cloud/internal/default_completion_queue_impl.cc 97.00% <0.00%> (-0.60%) ⬇️
.../cloud/storage/benchmarks/throughput_experiment.cc 74.37% <0.00%> (-0.51%) ⬇️
...le/cloud/spanner/database_admin_connection_test.cc 99.20% <0.00%> (-0.50%) ⬇️
...le/cloud/storage/internal/curl_download_request.cc 81.22% <0.00%> (-0.44%) ⬇️
...sub/internal/batching_publisher_connection_test.cc 97.60% <0.00%> (-0.21%) ⬇️
...cloud/pubsub/internal/subscription_session_test.cc 98.27% <0.00%> (+0.24%) ⬆️
google/cloud/bigtable/async_read_stream_test.cc 97.99% <0.00%> (+0.66%) ⬆️
google/cloud/grpc_error_delegate.cc 100.00% <0.00%> (+4.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ca03c6...2215b84. Read the comment docs.

@coryan coryan marked this pull request as ready for review July 19, 2021 11:19
@coryan coryan requested a review from a team July 19, 2021 11:19
dbolduc
dbolduc requested changes Jul 19, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@dbolduc dbolduc left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @coryan)

generator/integration_tests/generator_integration_test.cc, line 148 at r1 (raw file):

TEST_P(GeneratorIntegrationTest, CompareGeneratedToGolden) {
  auto golden_file = ReadFile(absl::StrCat(golden_path_, GetParam()));
  EXPECT_THAT(golden_file, IsOk());

in description you say we should use ASSERT_STATUS_OK(). Why not do it now?

generator/integration_tests/generator_integration_test.cc, line 152 at r1 (raw file):

      ReadFile(absl::StrCat(output_path_, product_path_, GetParam()));

  ASSERT_THAT(generated_file, IsOk());

same here

coryan
coryan commented Jul 19, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@coryan coryan left a comment

Choose a reason for hiding this comment

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

PTAL

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @dbolduc)

generator/integration_tests/generator_integration_test.cc, line 148 at r1 (raw file):

Previously, dbolduc (Darren Bolduc) wrote…

in description you say we should use ASSERT_STATUS_OK(). Why not do it now?

Done.

generator/integration_tests/generator_integration_test.cc, line 152 at r1 (raw file):

Previously, dbolduc (Darren Bolduc) wrote…

same here

ASSERT_THAT(..., IsOk()) is equivalent to ASSERT_STATUS_OK(...); 🤷

@google-cloud-cpp-bot
Copy link
Copy Markdown
Contributor

google-cloud-cpp-bot commented Jul 19, 2021

Google Cloud Build Logs
For commit: 2215b8427b58fbfadbae624ba15ba58bb327eaa3

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

dbolduc
dbolduc reviewed Jul 19, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@dbolduc dbolduc left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @dbolduc)

generator/integration_tests/generator_integration_test.cc, line 152 at r1 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

ASSERT_THAT(..., IsOk()) is equivalent to ASSERT_STATUS_OK(...); 🤷

Sorry, I misread what you said in the PR description.

@coryan coryan merged commit 790904f into googleapis:main Jul 19, 2021
@coryan coryan deleted the test-make-generator-test-more-readable branch July 24, 2021 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dbolduc dbolduc dbolduc approved these changes

+1 more reviewer

@devjgm devjgm devjgm approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@coryan @google-cloud-cpp-bot @devjgm @dbolduc