samples: Add BigTable delete samples#590
Conversation
- Add the deleteion snippets - Add test cases Closes internal issue #213629071
|
Here is the summary of changes. You are about to add 8 region tags.
This comment is generated by snippet-bot.
|
2ad0bec to
10dea88
Compare
a4bedb5 to
12c3e98
Compare
kolea2
left a comment
There was a problem hiding this comment.
lgtm, would also like @danieljbruce to take a look with @cshaff0524 :)
|
@kolea2 @cshaff0524 let me know if you have other concerns about this PR. Thanks. |
danieljbruce
left a comment
There was a problem hiding this comment.
Nicely done! I left a few comments/questions about how the framework is used and I left a few comments to make sure Python and Node.js line up.
|
|
||
|
|
||
| # [START bigtable_delete_from_column_sample] | ||
| def delete_from_column_sample(project_id, instance_id, table_id): |
| PROJECT, BIGTABLE_INSTANCE, table_id | ||
| ) | ||
| out, _ = capsys.readouterr() | ||
| snapshot.assert_match(out) |
There was a problem hiding this comment.
Since these two last lines of code are written 8 times, do you think we should refactor them out into a function and call them in each test case?
There was a problem hiding this comment.
Hmm. I see the reads_test doesn't do it that way though. It might be nice if all the samples were done this way, but I'm really curious about your thoughts.
|
|
||
| def test_delete_column_family(capsys, snapshot, table_id): | ||
| deletes_snippets.delete_column_family_sample(PROJECT, BIGTABLE_INSTANCE, table_id) | ||
| out, _ = capsys.readouterr() |
There was a problem hiding this comment.
I notice the snapshots at the end of the PR are empty. Ideally after each test I think we should print the data that remains in the table like we do in readSnippets. I think this just means adding the following code to the end of each function in delete_snippets.js.
for row in rows:
print_row(row)
There was a problem hiding this comment.
Yeah I had the snapshots to print things out at first, but I removed the prints. I think it was not necessary and noisy. I decided to go with calling the functions and checking that there is no errors.
|
The pre-release CI is not required (and still in the works) so we can ignore it. |
Closes internal issue #213629071
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #213629071 🦕