> A separate and second test, if we care, is to verify the sorting matches some predefined order
That is the way it works, currently. We have a test for a simple GET, some specific tests for filtering query parameters, one test checks pagination controls, another test checks some of orderby's functionality, and then one test exists only to test default ordering. > 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)? The sorting takes place in an "ORDER BY" clause of the SQL query that retrieves the Delivery Services from the database. The query is constructed in the endpoint handler in TO. This is done specifically and only so that the response to the client has a default ordering when the client specifies no other ordering. So really it depends what you mean by "authority" - sorting is done BY the database but it is "defined" by the way the handler builds the query, which in turn is built according to the URI of the client's request. On Tue, Jun 22, 2021 at 4:26 PM Taylor Frey <[email protected]> wrote: > 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 <[email protected]> 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 <[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". > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
