Skip to content

feat: Migrate sample and sample tests - Batch 2 of 5#12

Merged
leahecole merged 15 commits intosample-final-stagefrom
sample-staging-2
Nov 10, 2020
Merged

feat: Migrate sample and sample tests - Batch 2 of 5#12
leahecole merged 15 commits intosample-final-stagefrom
sample-staging-2

Conversation

@vinnysenthil
Copy link
Contributor

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:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #9 (Batch 2 of 5) 🦕

@vinnysenthil vinnysenthil requested review from a team and dizcology as code owners September 25, 2020 21:58
@vinnysenthil vinnysenthil requested review from leahecole and removed request for a team September 25, 2020 21:58
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 25, 2020
@release-please release-please bot requested a review from a team as a code owner September 25, 2020 23:09
Copy link
Contributor

@leahecole leahecole left a comment

Choose a reason for hiding this comment

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

I didn't go super in depth to each of them, but looked at the first three and found some common changes that I suspect are going to be common throughout

  • Avoid using globals, and instead emulate the pattern found in this dialogflow test for passing the variable to the fixture
  • Avoid testing the output of a fixture - keep your test logic to your tests. Instead, in the fixture, if you're worried about an operation failing or not being able to occur, utilize try/excepts (i.e., try to delete an endpoint, except notfound, print already deleted)
  • Make sure you do have an assertion based on the output in the test itself
  • Do there need to be as many print statements as there are in the samples, or is there a prettier way to get the user the info they need? (Either less info, or writing it to a file?) - this is the question I feel least strongly about and am inclined to trust you uCAIP folks

@product-auto-label product-auto-label bot added the samples Issues that are directly related to samples. label Sep 26, 2020
@vinnysenthil vinnysenthil changed the base branch from release-v0.2.0 to sample-final-stage September 30, 2020 23:41
Copy link
Contributor

@leahecole leahecole left a comment

Choose a reason for hiding this comment

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

Can you ensure that the sample names and the test names match? I'm guessing that create_training_pipeline_tabular_regression_sample and create_training_pipeline_tables_regression_test are related, as well as create_training_pipeline_tabular_classification_sample and create_training_pipeline_tables_classification_test

Unless I'm wrong and those are different in which case it seems like the Prs might be a little mixed up

Additionally, your tests are missing assertions entirely. You don't want them to be in the fixture, but you do want to make sure that you are making some assertions in the tests themselves

@vinnysenthil
Copy link
Contributor Author

Hi @leahecole - I've addressed requested changes:

  • Moved to using fixtures instead of mutable globals
  • Ensured assertions are made in every test
  • Got the filenames for tests + samples synced
  • Other changes to tests to get the passing, tests should successfully run in sample-final-stage once all five batch PRs are merged.

#11 has been merged which contains helpers.py. Merging the rest of the PRs in order

Copy link
Contributor

@leahecole leahecole left a comment

Choose a reason for hiding this comment

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

nit for all - direcotry reorg doesn't have to happen yet, but test names should match sample names, just with _test on the end :)

With some of the assertions, you are only looking for response - I want to double check that they won't give you a false positive if the response is blank/an error - double check that response is only going to happen if it for realz passes, and if not, adding another assertion to make it stronger

In some samples, you have a specific number of node hours declared - is this something that will be clear to the user? are these numbers arbitrary? if it's not going to be specified in accompanying docs, might be worth adding some comments, especially if these are decisions that will affect billing

@snippet-bot
Copy link

snippet-bot bot commented Nov 9, 2020

Here is the summary of changes.

You added 12 region tags.

To update this comment, add snippet-bot:force-run label.

@aribray aribray requested a review from leahecole November 10, 2020 00:40
@leahecole
Copy link
Contributor

approving and merging to next stage despite test failures - no test failures when we go to master though :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement. samples Issues that are directly related to samples.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants