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

Don't assume order when testing links to changeset elements #4571

Conversation

AntonKhorev
Copy link
Collaborator

I noticed that one test fails sometimes:

Failure:
ChangesetsControllerTest#test_show_paginated_element_links [test/controllers/changesets_controller_test.rb:312]:
Expected exactly 1 element matching "a[href='/way/109']", found 0.
Expected: 1
  Actual: 0

bin/rails test test/controllers/changesets_controller_test.rb:301

That's probably because I assumed that on the changeset page the elements will be shown in the same order as they were created. But actually they are not sorted, so sometimes the order may be different.

Here I remove the ordering assumption and check the next page for an element that was missing from the first page.

@gravitystorm
Copy link
Collaborator

But actually they are not sorted, so sometimes the order may be different.

Good catch.

Would it be a better idea for us to sort the elements instead? I've had a brief look at the website, and it would seam reasonable to sort the elements by id and version. That way the order is stable, and also might be helpful for mappers too (e.g I think if I saw nodes 1, 2, 5, ..., 20 on the first page, I'd be surprised to find node 4 mentioned on page 7).

I also think the test could be improved. At the moment the test creates 21 items for each type of element, and then checks that the first 20 are visible. But it doesn't check that the 21st item is not visible, and it doesn't check that there are page links to view the next page. I can see what you are doing with the changes that you propose, but I think that leads to a very complex test, in order to handle a situation (lack of sort order) that I don't think is intentional.

@AntonKhorev
Copy link
Collaborator Author

@AntonKhorev
Copy link
Collaborator Author

it would seam reasonable to sort the elements by id and version

Another reasonable order is by modification time, which is what usually happens already.

@gravitystorm
Copy link
Collaborator

It does:
https://github.com/openstreetmap/openstreetmap-website/pull/4571/files#diff-6cdb8b8fa44e422834c4815943a8e4933d6d09e4687f338d007daeff1a90daa9R322

I'm sorry I wasn't clear - I was commenting on the original test. I meant that even if we establish a sort order, the test could still benefit from improvements.

@AntonKhorev
Copy link
Collaborator Author

Changeset download sorts elements by (timestamp, version):
8a57904

@AntonKhorev
Copy link
Collaborator Author

@tomhughes
Copy link
Member

I'm with @gravitystorm that this is the wrong fix - aside from the horrendous complexity involved in this fix it's fundamentally wrong to try and do pagination without a fixed order.

Without a sort you have no guarantee that the database will use the same order for each page so you may wind up seeing some things twice and not seeing others at all as you move through the pages.

@AntonKhorev
Copy link
Collaborator Author

15 years of wrong pagination 2ed84e2

@AntonKhorev
Copy link
Collaborator Author

Not needed if #5209 is merged.

@AntonKhorev AntonKhorev deleted the fix-test_show_paginated_element_links branch September 18, 2024 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dx Developer Experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants