Seems like we may have already come to a consensus, but I wanted to throw my 2 cents in real quick.
TLDR; One concept per test. No to enforcing a locale. Fix the test to validate the content and remove any test verification that has to do with the order returned. In general, I try to abide by Uncle Bob's "One concept per test". While specifically detailed in his Unit Test section, I believe it's applicable for all tests. In this case we may have accidentally conflated two concepts when porting to Go by both testing the functionality of the GET *and* the ordering, when they should really be two distinct tests. One test should validate the functionality, likely by matching the content returned w/o care of order. A separate and second test, if we care, is to verify the sorting matches some predefined order (which I would probably say we do not want to verify at all). Whether we ultimately verify the sort order, or not, it should be in a separate test anyways. I suppose the last point/question I would have is which layer is the sorting authority? Is the sorting defined by the DB / PostgreSQL, Go, or the client like Traffic Portal tables/grids (Granted this is in the API tests)? T On Mon, Jun 21, 2021 at 9:04 PM ocket 8888 <ocket8...@gmail.com> wrote: > That works for me. Models the intent and the test isn't actually any > weaker, just in a different place. > > On Mon, Jun 21, 2021 at 4:10 PM Rawlin Peters <raw...@apache.org> 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 <zrhoff...@apache.org> > 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 <ocket8...@gmail.com> > 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 <hoffz...@gmail.com> > > 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 <ocket8...@gmail.com> > > 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 <ocket8...@gmail.com> > > > > 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". > > > > > > > > > > > > > > > > > > > > > > > > >
