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

examples: Add _helpers.py file to non-driver examples and update acceptance tests #329

Merged
merged 15 commits into from
Jul 22, 2023

Conversation

vigkre
Copy link
Collaborator

@vigkre vigkre commented Jul 12, 2023

What does this Pull Request accomplish?

  • Adds the common helpers file _helpers.py to the example measurements:
    • sample_measurement
    • sample_streaming_measurement
    • ui_progress_updates

Why should this Pull Request be merged?

  • When customer tries to generate a measurement using template, they will be able to notice _helpers.py file generated, but when they refer the example measurements they might not see _helpers.py file.
  • To overcome this confusion, driver examples (nidcpower, nidmm etc..,) are already updated with _helpers.py file to match template. Hence, updating the other examples to use _helpers.py
  • Adds fake measurement_service (copied details from example measurements).
  • Update tests to use fake measurement_service instead of running examples

What testing has been done?

  • Updated the examples to use _helpers.py and Ran poetry run pytest to verify all tests passed.

Driver examples are updated with _helpers.py file to match template

Hence updating the other examples to use _helpers.py

Signed-off-by: Vikram Avudaiappan <vikram.avudaiappan@ni.com>
@github-actions
Copy link

github-actions bot commented Jul 12, 2023

Test Results

     12 files  ±0       12 suites  ±0   17m 32s ⏱️ - 1m 14s
   180 tests ±0     180 ✔️ ±0  0 💤 ±0  0 ±0 
2 148 runs  ±0  2 148 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 54fe38e. ± Comparison against base commit ef7d247.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@dixonjoel dixonjoel left a comment

Choose a reason for hiding this comment

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

I could go either way on whether we should include these, but I have no strong objection to doing it. I'd wait for @bkeryan to chime in before submitting.

@bkeryan
Copy link
Collaborator

bkeryan commented Jul 12, 2023

I could go either way on whether we should include these, but I have no strong objection to doing it. I'd wait for @bkeryan to chime in before submitting.

Updating these examples to use _helpers.py will break the tests. See #257

If you want to use _helpers.py in all the examples, you should update the tests to use their own services instead of running the examples.

@dixonjoel
Copy link
Collaborator

Updating these examples to use _helpers.py will break the tests. See #257

All the checks passed for this PR. Did you expect them to break? In what way does _helpers.py cause them to break?

@bkeryan bkeryan mentioned this pull request Jul 12, 2023
1 task
@bkeryan
Copy link
Collaborator

bkeryan commented Jul 12, 2023

Updating these examples to use _helpers.py will break the tests. See #257

All the checks passed for this PR. Did you expect them to break? In what way does _helpers.py cause them to break?

This PR doesn't update the examples to use _helpers.py. It just adds dead code.

If you update measurement.py to import _helpers, it will fail when you run the tests because there is no top-level _helpers module when you run the tests. If you update measurement.py to import helpers in a way that works in the tests, then it will fail when you run the measurement standalone. See #328 (comment) for more info.

@vigkre vigkre changed the title examples: Add _helpers.py file to non-driver examples examples: Add _helpers.py file to non-driver examples and update acceptance tests Jul 17, 2023
Copy link
Collaborator Author

@vigkre vigkre left a comment

Choose a reason for hiding this comment

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

If you want to use _helpers.py in all the examples, you should update the tests to use their own services instead of running the examples.

@bkeryan I have added fake measurement_service (copied details from example measurements). Also, update acceptance tests to use fake measurement_service instead of running examples.

tests/utilities/fake_measurement_service.py Outdated Show resolved Hide resolved
tests/utilities/fake_measurement_service.py Outdated Show resolved Hide resolved
tests/utilities/fake_measurement_service.py Outdated Show resolved Hide resolved
examples/sample_measurement/_helpers.py Outdated Show resolved Hide resolved
@vigkre vigkre requested a review from bkeryan July 19, 2023 04:59
@vigkre vigkre requested a review from bkeryan July 21, 2023 19:47
@vigkre vigkre merged commit 942483b into main Jul 22, 2023
17 checks passed
@dixonjoel dixonjoel deleted the users/vikram/add-helpers-non-driver-examples branch August 16, 2023 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants