Skip to content

Commit aa5259b

Browse files
gmorador-tribuElimpizza
authored andcommitted
Order the recipients of the instructor email digests
The query which picks the instructors to email had no `ORDER BY`, so Postgres returned the rows in whatever physical order it found them in, which depends on what else had been written to `user` before. That made `test_it_sends_digests_for_instructors` pass or fail by luck: it asserts the calls come out sorted by `h_userid`, which only held when the heap happened to be laid out that way. The unit tests all share one database (`tox.ini` points every worker at `lms_tests`) and run under `--dist loadgroup`, so which tests a worker picks up — and therefore what was written to `user` first — depends on the collected test list. Adding the two dashboard API tests in this branch was enough to reshuffle it and turn the test red, with no change to the code it covers. Ordering in the query rather than in the test also makes the order the digests go out in deterministic in production, which it was not.
1 parent 2c03362 commit aa5259b

1 file changed

Lines changed: 9 additions & 0 deletions

File tree

lms/tasks/email_digests.py

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,15 @@ def send_instructor_email_digest_tasks():
100100
.is_(False)
101101
),
102102
)
103+
# Without this the rows come back in whatever physical order
104+
# Postgres finds them in, which depends on what else has been
105+
# written to `user` before. That makes the order the digests go
106+
# out in arbitrary, and it makes the test which asserts the
107+
# recipients pass or fail depending on which xdist worker picked
108+
# it up: the unit tests all share one database, so any change to
109+
# the number of tests reshuffles the distribution and can flip
110+
# it.
111+
.order_by(User.h_userid)
103112
).all()
104113

105114
for h_userid in h_userids:

0 commit comments

Comments
 (0)