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

Use xfail consistently for known failing tests #867

Merged
merged 36 commits into from Jan 10, 2023

Conversation

charlesbluca
Copy link
Collaborator

@charlesbluca charlesbluca commented Oct 18, 2022

The intent of this PR is to make the way we indicate failing tests more consistent, by:

  • uncommenting any tests that have been commented out in their entirety, xfailing with a reason if they fail
  • uncommenting any sections of tests that are partially failing, splitting these sections off into separate tests if they fail

Closes #899

@codecov-commenter
Copy link

codecov-commenter commented Oct 24, 2022

Codecov Report

Merging #867 (ee0e689) into main (fa36ac0) will increase coverage by 2.93%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #867      +/-   ##
==========================================
+ Coverage   77.69%   80.63%   +2.93%     
==========================================
  Files          75       75              
  Lines        4218     4218              
  Branches      766      766              
==========================================
+ Hits         3277     3401     +124     
+ Misses        775      651     -124     
  Partials      166      166              
Impacted Files Coverage Δ
dask_sql/context.py 94.21% <0.00%> (ø)
dask_sql/datacontainer.py 97.14% <0.00%> (+0.95%) ⬆️
dask_sql/_version.py 35.31% <0.00%> (+1.41%) ⬆️
dask_sql/physical/rel/logical/aggregate.py 91.11% <0.00%> (+1.66%) ⬆️
dask_sql/input_utils/convert.py 88.23% <0.00%> (+5.88%) ⬆️
dask_sql/input_utils/location.py 92.00% <0.00%> (+12.00%) ⬆️
dask_sql/server/app.py 100.00% <0.00%> (+65.15%) ⬆️
dask_sql/server/responses.py 97.87% <0.00%> (+71.27%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@charlesbluca charlesbluca marked this pull request as ready for review October 25, 2022 15:28
@charlesbluca charlesbluca mentioned this pull request Nov 2, 2022
@charlesbluca charlesbluca marked this pull request as draft December 13, 2022 20:28
Comment on lines +7 to +8
# FIXME: handling is needed for httpx-based fastapi>=0.87.0
- fastapi>=0.69.0,<0.87.0
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fastapi>=0.87.0 pulls in starlette>=0.21.0, which switched to using httpx instead of requests for its TestClient API (encode/starlette#1376); it's not immediately obvious to me how to support both versions of the API, so this pins us to the older starlette packages for now.

@@ -332,15 +332,15 @@ def gpu_client(gpu_cluster):
# client for all computations. otherwise, only connect to a client
# when specified.
@pytest.fixture(
scope="function" if SCHEDULER_ADDR is None else "session",
scope="function",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When we use a session-wide client for external cluster testing, it ends up getting blown away with dask.config.refresh() while running test_config.py. Since there wasn't any immediate way to avoid this, I made this change so that we reconnect to the external cluster on each test, so that tests run after test_config.py are still able to use the external cluster as expected.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I also wonder if test_config can be improved somehow to save the state of configs before the tests start and restore that state at the end of the test.

Either way I didn't see a noticeable change in overall test runtimes with this change so it might be okay to improve later.

@charlesbluca charlesbluca marked this pull request as ready for review January 10, 2023 03:06
Copy link
Collaborator

@ayushdg ayushdg left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this overhaul of the pytests and moving over to marking everything as xfail and removing commented subparts of tests into their own xfailing test. This will help us catch bugs and issues much quicker.

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.

[BUG] Case when types incorrect in certain cases
3 participants