Rawlin's idea of asserting the query is ordered sounds solid, this will keep our test coverage without pinning us to a specific testing environment.
-Zach On Mon, Jun 21, 2021 at 4:10 PM Rawlin Peters <[email protected]> wrote: > 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". > > > > > > > > > > > > > > > > > > >
