alastairp: what do you think is a good way to handle the above issue in the test.sh script. The test.sh script currently exits with a ) exit code even if the tests fails. I was thinking to use `set -e` but that doesn't work because we use functions that return non-zero values eg: `is_unit_db_running`. Other option is to store the exit code of the command that ran tests in test.sh and then return it at the end of the script.
2021-04-19 10935, 2021
_lucifer
This looks fine to me. Do you think there's a better a solution?
2021-04-19 10902, 2021
alastairp
_lucifer: hi, I saw that message last week
2021-04-19 10948, 2021
alastairp
does test.sh run multiple tests in the same job? (e.g. frontend runs 3 things in a row, right?
2021-04-19 10902, 2021
alastairp
so if the first one fails, will the other two run?
2021-04-19 10915, 2021
_lucifer
No, we call it thrice to do three different things.
and do we try and do cleanup at the end of the job?
2021-04-19 10904, 2021
_lucifer
test.sh handles that
2021-04-19 10922, 2021
alastairp
even if you set -e ?
2021-04-19 10916, 2021
_lucifer
actually set -e doesn't work here as i mentioned above. my intent was set -e then even if cleanup doesn't happen in the CI its fine because the container will just go away.
2021-04-19 10950, 2021
alastairp
yeah, right
2021-04-19 10902, 2021
_lucifer
when is_unit_db_running or any other function we define in the script returns a non zero value it exits.
2021-04-19 10920, 2021
_lucifer
so we'll have to handle it manually i think.
2021-04-19 10925, 2021
alastairp
I was going to suggest one option would be to use exec, that makes docker-compose take over the scrit, and its return value will be the return value
2021-04-19 10938, 2021
alastairp
but that means you won't be able to do a cleanup after
2021-04-19 10957, 2021
_lucifer
that's an issue when we run locally.
2021-04-19 10901, 2021
alastairp
yeah
2021-04-19 10919, 2021
alastairp
so, you were thinking of catching the return value of docker-compose, and then returning that at the very end of the script?
2021-04-19 10934, 2021
_lucifer
yes
2021-04-19 10900, 2021
_lucifer
one issue is that we lose the return value of the cleanup but i see we are not using it anyway, we just exit 0.
2021-04-19 10912, 2021
alastairp
I don't see a problem with not having the return value of the cleanup
2021-04-19 10955, 2021
_lucifer
cool, let's do that then catch the return value and return it at end.
2021-04-19 10951, 2021
alastairp
even though it's a bit annoying I still think it's the better way to go, using test.sh in the action
2021-04-19 10956, 2021
_lucifer
yeah, also we'll probably replace it with a test.py soon which should be easier to work with IMO.
2021-04-19 10914, 2021
outsidecontext
Nena should have sung about "100 Luftballons", then it wouldn't fool Picard's guess track number from filename :(
in get_or_create() I would like to set default mint_ts and max_ts timestamps for a user into redis, but only if the user is new. but in the db.user.py module is the wrong place to do it (logically, and also importing the timescale listenstore yields an import circular dependency. so it should be done in listenbrainz/webserver/login/provider.py
2021-04-19 10937, 2021
_lucifer
ruaok, python packages set up on new cluster. dataframes generated successfully. there is an incompatitble change wrt to schema in spark 3.* which needs to be fixed before recommendations start working. currently working on that.
2021-04-19 10942, 2021
ruaok
but, there I can't tell if the user was created or not -- could return a tuple of (user, created), but our tests call this function 94 times, which makes the PR much larger and more noise in our codebase.
2021-04-19 10957, 2021
ruaok
thoughts for how to best to this iliekcomputers _lucifer alastairp ?
2021-04-19 10924, 2021
ruaok
_lucifer: thanks. this is all perfect timing, I hope to get recommendations working again this week.
2021-04-19 10946, 2021
alastairp
ruaok: sorry, was in a call and didn't see your first question
2021-04-19 10952, 2021
ruaok
no worries.
2021-04-19 10919, 2021
alastairp
in django, their version of get_or_create returns a tuple (object, created), where you can check this
2021-04-19 10931, 2021
alastairp
but as you point out, the method is already used in a bunch of places
2021-04-19 10946, 2021
ruaok
94 more changes to the PR would suck.
2021-04-19 10942, 2021
alastairp
oh, you already said that. missed that part of the line
2021-04-19 10943, 2021
ruaok
I could just add another get_by_mb_row_id call to check to see if the users exists. if it is fetched, the next call should effectively be cached in PG and return quickly.
2021-04-19 10907, 2021
_lucifer
if the only issue is that, we could do it in a separate PR and merge that first.
2021-04-19 10915, 2021
alastairp
just loading code up now
2021-04-19 10952, 2021
ruaok
_lucifer: yeah, I'd honestly prefer to do it in different way and not clutter up our tests more than needed.
2021-04-19 10924, 2021
alastairp
(there are also 34 instances of `create` in tests which for some reason don't use get_or_create :|)
2021-04-19 10927, 2021
alastairp
ruaok: hmmm
2021-04-19 10949, 2021
ruaok
indeed.
2021-04-19 10918, 2021
alastairp
if you did it in provider.get_user, you could get, if it's none set a sentinel, do get or create, then if it was created, set the redis stuff
2021-04-19 10922, 2021
alastairp
that's what you suggested?
2021-04-19 10902, 2021
ruaok
yeah
2021-04-19 10907, 2021
alastairp
yea, fine by me
2021-04-19 10913, 2021
ruaok
k, less mess.
2021-04-19 10915, 2021
alastairp
one potential problem there is that this is only used from the web login flow (right?). if we need this data in redis for an integration test, we might have to create it manually, which then means we might need 2 ways of making the same data
alastairp: I dont think we need an integration test -- I'll add a regular one and should be fine.
2021-04-19 10915, 2021
ruaok
meh, we have no tests at all for testing the login process.
2021-04-19 10933, 2021
_lucifer
a good time to do that when we do account migration
2021-04-19 10941, 2021
ruaok
agreed.
2021-04-19 10928, 2021
alastairp
ruaok: right, I didn't specifically mean that we need an integration test for this. I mean, if you have an integration test that requires reading these values from redis, how are you going to set them?
2021-04-19 10944, 2021
ruaok
by calling the cache.put -- afterall I am only inserting (0,0)
2021-04-19 10922, 2021
ruaok
the idea is this: if a user creates an account and then goes to their listens page, that would causes us to do an INDEX scan on the whole DB, with the guarntee of not finding anything.
2021-04-19 10948, 2021
ruaok
but inserting (0,0) for the timestamps, means that the timescale listenstore knowns there are no listens and does not query.
2021-04-19 10921, 2021
alastairp
ok, good
2021-04-19 10931, 2021
_lucifer
these keys will not be expiring automatically?
2021-04-19 10945, 2021
CatQuest has quit
2021-04-19 10908, 2021
CatQuest joined the channel
2021-04-19 10908, 2021
CatQuest has quit
2021-04-19 10908, 2021
CatQuest joined the channel
2021-04-19 10929, 2021
ruaok
no, they are now intended to be in redis at all times without expiry. we now update the keys when the DB values change.
2021-04-19 10946, 2021
ruaok
in fact this is the only reason why listen counts were ever accurate in the first place, lol. ;)
2021-04-19 10929, 2021
_lucifer
cool, you might want to pass `time=0` manually then
2021-04-19 10915, 2021
_lucifer
we'll be updating BU sometime soon so that the caller has to always specify a timeout. `time=0` if you do not want expiry.
2021-04-19 10929, 2021
ruaok
oh, that's important.
2021-04-19 10906, 2021
_lucifer
New recommendations generated.
2021-04-19 10924, 2021
ruaok
:D
2021-04-19 10925, 2021
alastairp
this is a good place to remind us that we had this open ticket about separating the redis interface in BU into cache (epheremal, may disappear) and data (need to keep around permanently), and if we should have 2 different interfaces for that task [not suggesting we do it now, let's just use cache, but something to keep in mind for future improvements]
