I think the intent was just to provide some kind of default ordering if no `orderby` query parameter was passed in the request. This was what TO-Perl did originally, but it was lost in the transition to Go at first. It was added later to get back to parity with the original TO-Perl version, since there were lots of APIs used by Traffic Portal that relied on the default ordering for usability. When the default ordering was lost, a lot of lists became unsorted, making them harder to select options from.
Maybe we should remove the TO API tests that test the resulting sort when using (or not using) the `orderby` param, and instead make them unit tests that check that the expected `ORDER BY ...` clause is added to the resulting DB query. Then the tests are no longer locale-dependent, but we're still testing the expected behavior. - Rawlin On Mon, Jun 21, 2021 at 3:51 PM Zach Hoffman <[email protected]> wrote: > > How would removing or modifying the test change our response to a user > saying "Sorting doesn't look right"? > > -Zach > > On Mon, Jun 21, 2021 at 3:48 PM ocket 8888 <[email protected]> wrote: > > > But if we test our sorting according to a certain locale, our response to > > "sorting doesn't look right" is going to be "what's your locale?" instead > > of "what's wrong?". If the behavior as tested only works in one locale, > > that's the only locale it supports. This is why I think tests are a > > statement of intent about the behavior under test. I'm fine with a weaker > > test if it better represents what we actually want. > > > > On Mon, Jun 21, 2021 at 1:46 PM Zach Hoffman <[email protected]> wrote: > > > > > > I don't feel good about telling the community/world "this software only > > > runs on machines configured for United States-flavor English". > > > > > > We shouldn't need to tell the community/world "this software only runs on > > > machines configured for United States-flavor English," we can just > > document > > > that a specific locale is required for running the API tests. > > > > > > -Zach > > > > > > On Mon, Jun 21, 2021 at 1:40 PM ocket 8888 <[email protected]> wrote: > > > > > > > > I think the likelihood of that test passing when underlying > > > functionality > > > > *IS BROKEN* can be made acceptably low... > > > > > > > > clarification > > > > > > > > On Mon, Jun 21, 2021 at 1:35 PM ocket 8888 <[email protected]> > > wrote: > > > > > > > > > Currently, as some of you may have noticed, the integration tests for > > > > > Traffic Ops and its Go API client are failing on master. This is > > > > > (partially) because of a test that checks that the response to a GET > > > > > request to /deliveryservices at version 4.0 is sorted by XMLID if no > > > > > orderby query string parameter is provided. The way it does this is > > by > > > > > making the request, then sorting the returned XMLIDs, then comparing > > > the > > > > > actual response to the sorted list. > > > > > > > > > > Here's the problem: the ordering is done in an SQL "ORDER BY" clause, > > > > > which - besides being a different runtime - runs on ostensibly an > > > > entirely > > > > > separate system from the client/TO integration tests. If those two > > > > systems > > > > > aren't using the same locale, they won't sort the same set of strings > > > in > > > > > the same way. Specifically, right now our tests use a C locale, which > > > > sorts > > > > > "-" *after* "1", but the postgresql service uses en_US.utf8, which > > > sorts > > > > > "-" *before* "1". > > > > > > > > > > This is the tests passing (that one, specifically, some vault things > > > are > > > > > still failing) after add a `COLLATE "C"` statement to the query's > > > "ORDER > > > > > BY" clause: > > > > > > > > > > > > > > https://github.com/apache/trafficcontrol/pull/5957/checks?check_run_id=2878328870 > > > > > > > > > > This is the tests failing without that extra statement: > > > > > https://github.com/apache/trafficcontrol/runs/2853688690 > > > > > > > > > > Now: why is this a mailing list matter? Because, all of our laptops > > are > > > > > using en_US.utf8, or somebody would've said something way earlier. > > And > > > > > every attempt I've made (in this PR: > > > > > https://github.com/apache/trafficcontrol/pull/5957) to change the > > > locale > > > > > in which the tests are running has failed. So adding that locale > > > > statement > > > > > will cause the tests to start failing without extra configuration on > > > > > everyone's local development machines and in every existing CI > > > > environment > > > > > besides GHA, and we apparently can't force the GHA tests to comply > > with > > > > our > > > > > existing environments (I think - if I've missed something somebody > > > please > > > > > let me know). > > > > > > > > > > To solve this, I can think of three options: > > > > > > > > > > 1. Get rid of the test. Default ordering is undocumented, so nobody > > > > should > > > > > actually be relying on it. > > > > > 2. Change the test; the original purpose behind default ordering was > > to > > > > > make requests deterministic so that with the same data in TO you > > would > > > > > always get the same response. The test can't be absolutely sure that > > if > > > > it > > > > > checks N times that if it did it N+1 times the consistency wouldn't > > be > > > > > broken, but we can ensure that under testing conditions that making, > > > > say, 3 > > > > > identical, subsequent requests with identical data in TO yields > > > identical > > > > > responses. It's a weaker test, but it could be said it's better than > > > > > nothing. > > > > > 3. Enforce a locale. If the actual order is important to us, then we > > > must > > > > > enforce a locale, because that defines the order. en_US.UTF-8 seems a > > > > > logical choice, so that the only thing that needs to change is the > > > test. > > > > It > > > > > will mean, materially, writing our own sorting function that uses > > > > > en_US.UTF-8 rules to compare bytes in a string - regardless of the > > > > > execution environment's locale - and adding some documentation that > > > notes > > > > > that our supported environments are restricted to a specific locale. > > > > > > > > > > Personally, I like option 2. I think our tests ought to reflect the > > > > > intended behavior, not implementation details. I think the likelihood > > > of > > > > > that test passing when underlying functionality can be made > > acceptably > > > > low, > > > > > and I think that in general the locale of a system is not something a > > > > > single piece of software running on it should dictate. I don't feel > > > good > > > > > about telling the community/world "this software only runs on > > machines > > > > > configured for United States-flavor English". > > > > > > > > > > > > > >
