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

re-add quick check for alternative engineering database test #7780

Merged
merged 1 commit into from
Aug 16, 2023

Conversation

braingram
Copy link
Collaborator

@braingram braingram commented Jul 28, 2023

The test_environmental test when not run on the vpn will fail to contact the alternative engineering database server. This results in the test taking ~10 minutes waiting for a response before turning into a skip.

This commit adds some lines of code that were removed in PR #3022 which changed the skip from a decorator to a pytest.skip when the exception/timeout occurs. This should allow the test to skip quickly when the host is not accessable (and instead skips in <1 second). The pytest.skip call during the except block is left to deal with cases when the host resolves but the url fails to resolve.

As the alternative host appears unresponsive for github CI runs (which run tests in parallel with 2 workers) this should free up one of the workers that without this PR sits waiting for ~10 minutes. As a comparison, the python 3.9 tests for this recent PR took ~22 minutes whereas similar tests for this PR took ~16 minutes.

Resolves JP-nnnn

Closes #

This PR addresses ...

Checklist for maintainers

  • added entry in CHANGES.rst within the relevant release section
  • updated or added relevant tests
  • updated relevant documentation
  • added relevant milestone
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below.
    How to run regression tests on a PR
  • Make sure the JIRA ticket is resolved properly

@braingram
Copy link
Collaborator Author

@stscieisenhamer as the author of #3022 do you foresee issues with reintroducing the decorator to skip this test when the host does not respond? If so, are there alternatives you might suggest for allowing this test to skip without waiting for the timeout. Perhaps skipping it for github CI runs (although this also impacts local runs when not on the vpn) or reducing the timeout?

@braingram braingram force-pushed the alt_engr_host_check branch from e9ed98f to 1170fa9 Compare July 28, 2023 19:43
@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.04% 🎉

Comparison is base (6d6d1b2) 76.54% compared to head (1170fa9) 76.59%.

❗ Current head 1170fa9 differs from pull request most recent head c8f202f. Consider uploading reports for the commit c8f202f to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7780      +/-   ##
==========================================
+ Coverage   76.54%   76.59%   +0.04%     
==========================================
  Files         456      456              
  Lines       36947    36963      +16     
==========================================
+ Hits        28282    28310      +28     
+ Misses       8665     8653      -12     
Flag Coverage Δ *Carryforward flag
nightly 77.42% <ø> (+0.03%) ⬆️ Carriedforward from 96247f1

*This pull request uses carry forward flags. Click here to find out more.

see 9 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@braingram braingram force-pushed the alt_engr_host_check branch from 1170fa9 to aad4c6f Compare August 2, 2023 16:45
@braingram
Copy link
Collaborator Author

@braingram braingram marked this pull request as ready for review August 2, 2023 16:47
@braingram braingram requested a review from a team as a code owner August 2, 2023 16:47
@braingram
Copy link
Collaborator Author

Closing and reopening to retrigger docs.

@braingram braingram closed this Aug 2, 2023
@braingram braingram reopened this Aug 2, 2023
@braingram
Copy link
Collaborator Author

Not sure why the docs are failing with:

Read the Docs build information
Build id: 21486088
Project: jwst-pipeline
Version: 7780
Commit: aad4c6f4321bf62c286d14fc9a199e8e374df3a4
Date: 2023-08-02T16:48:38.406583Z
State: finished
Success: False


[rtd-command-info] start-time: 2023-08-02T16:48:41.720584Z, end-time: 2023-08-02T16:48:44.742024Z, duration: 3, exit-code: 0
git clone --depth 1 https://github.com/spacetelescope/jwst .
Cloning into '.'...

[rtd-command-info] start-time: 2023-08-02T16:48:45.113008Z, end-time: 2023-08-02T16:48:46.048983Z, duration: 0, exit-code: 0
git fetch origin --force --prune --prune-tags --depth 50 pull/7780/head:external-7780
From https://github.com/spacetelescope/jwst
 * [new ref]         refs/pull/7780/head -> external-7780
 * [new tag]         1.11.0              -> 1.11.0

[rtd-command-info] start-time: 2023-08-02T16:48:46.264652Z, end-time: 2023-08-02T16:48:46.359021Z, duration: 0, exit-code: 128
git checkout --force aad4c6f4321bf62c286d14fc9a199e8e374df3a4
fatal: reference is not a tree: aad4c6f4321bf62c286d14fc9a199e8e374df3a4

@zacharyburnett Have you seen any error like this before?

@braingram braingram force-pushed the alt_engr_host_check branch from aad4c6f to e7914aa Compare August 2, 2023 16:55
@braingram braingram closed this Aug 2, 2023
@braingram braingram reopened this Aug 2, 2023
@braingram
Copy link
Collaborator Author

The docs seem happy again. I'm going to chalk this up to readthedocs attempting to clone the repo before github was fully updated or something.

@braingram
Copy link
Collaborator Author

Regression tests passed with no failures.

Note that the modified test test_environmental passed during the regression test run in 3 seconds (so it did not time out and did not skip itself).

@stscieisenhamer stscieisenhamer self-requested a review August 15, 2023 13:30
@braingram braingram force-pushed the alt_engr_host_check branch from e7914aa to f007523 Compare August 15, 2023 13:33
@braingram
Copy link
Collaborator Author

braingram commented Aug 15, 2023

Thanks! I rebased this PR to grab the most recent changes.

The test_environmental test when not run on the vpn
will fail to contact the alternative engineering database
server. This results in the test taking ~10 minutes waiting
for a response before turning into a skip.

This commit adds some lines of code that were removed in
PR spacetelescope#3022 which changed the skip from a decorator to a
pytest.skip when the exception/timeout occurs. This should
allow the test to skip quickly when the host is not accessable
(and instead skips in <1 second). The pytest.skip call
during the except block is left to deal with cases when the
host resolves but the url fails to resolve.
@braingram braingram force-pushed the alt_engr_host_check branch from f007523 to c8f202f Compare August 15, 2023 13:34
@nden nden added this to the Build 10.0 milestone Aug 16, 2023
@nden nden merged commit c86ff0e into spacetelescope:master Aug 16, 2023
@braingram braingram deleted the alt_engr_host_check branch August 16, 2023 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants