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

table_config instead of table_metadata #2257

Merged
merged 3 commits into from
Feb 7, 2024
Merged

table_config instead of table_metadata #2257

merged 3 commits into from
Feb 7, 2024

Conversation

simonw
Copy link
Owner

@simonw simonw commented Feb 7, 2024

@simonw simonw added the refactor label Feb 7, 2024
@simonw simonw added this to the Datasette 1.0a8 milestone Feb 7, 2024
@simonw
Copy link
Owner Author

simonw commented Feb 7, 2024

Two failing tests (locally):

FAILED tests/test_api.py::test_database_page - AssertionError: assert equals failed
FAILED tests/test_facets.py::test_conflicting_facet_names_json - AssertionError: assert equals failed

The test_database_page problem boils down to this:

CleanShot 2024-02-06 at 21 24 22@2x

It's a hidden: False v.s. hidden: True thing.

The other one seems to be intermittent? I'll fix it after this first one

@simonw
Copy link
Owner Author

simonw commented Feb 7, 2024

Yeah no_primary_key should be hidden=True - according to this:

"no_primary_key": {"sortable_columns": [], "hidden": True},

Which is part of METADATA= in fixtures.py.

Note that this has moved to config - but I have new code that copies that value over from metadata so it should continue to work.

@simonw
Copy link
Owner Author

simonw commented Feb 7, 2024

Running that failing test on its own passes:

just test -k test_conflicting_facet_names_json
==== test session starts ====
platform darwin -- Python 3.10.13, pytest-7.2.1, pluggy-1.0.0
SQLite: 3.44.0
rootdir: /Users/simon/Dropbox/Development/datasette, configfile: pytest.ini
plugins: icdiff-0.6, asyncio-0.20.3, xdist-3.1.0, timeout-2.1.0, anyio-3.6.2
asyncio: mode=strict
collected 1434 items / 1433 deselected / 1 selected                                                                                                      

tests/test_facets.py .                                                                                                                             [100%]
==== 1 passed, 1433 deselected in 1.20s ====

@simonw
Copy link
Owner Author

simonw commented Feb 7, 2024

Now I get these two failures locally:

================================================================ short test summary info =================================================================
FAILED tests/test_facets.py::test_conflicting_facet_names_json - AssertionError: assert equals failed
FAILED tests/test_table_api.py::test_paginate_tables_and_views[/fixtures/paginated_view.json-201-9] - assert 9 == 5
============================================ 2 failed, 1429 passed, 2 skipped, 1 xfailed in 70.64s (0:01:10) =============================================

@simonw
Copy link
Owner Author

simonw commented Feb 7, 2024

This seems bad: specifying those tests individually causes them to pass.

I'm worried I've got some state leaking between tests somehow.

@simonw
Copy link
Owner Author

simonw commented Feb 7, 2024

I bet it's because move_table_config(metadata, config) modifies those dictionaries in-place.

Comment on lines +1310 to +1311
source = copy.deepcopy(source)
destination = copy.deepcopy(destination)
Copy link
Owner Author

Choose a reason for hiding this comment

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

I'm a bit worried about the performance overhead of this - not on Datasette itself (it only starts up once) but on the Datasette test suite which might call this hundreds of times.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I'm going to ignore this problem in the interest of getting this release done.

@simonw simonw marked this pull request as ready for review February 7, 2024 05:55
Copy link

codecov bot commented Feb 7, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (52a1dac) 92.46% compared to head (b239ac6) 92.53%.

Files Patch % Lines
datasette/utils/__init__.py 96.29% 1 Missing ⚠️
datasette/views/table.py 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2257      +/-   ##
==========================================
+ Coverage   92.46%   92.53%   +0.07%     
==========================================
  Files          42       42              
  Lines        6290     6310      +20     
==========================================
+ Hits         5816     5839      +23     
+ Misses        474      471       -3     

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

@simonw simonw merged commit 60c6692 into main Feb 7, 2024
19 checks passed
@simonw simonw deleted the table-config branch February 7, 2024 05:57
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.

1 participant