[05:44] hi! [06:07] morning [06:07] mardy: hey [06:20] morning [06:28] pstolowski: hi [06:28] mardy: hi! well played, congrats! ;) [06:57] pstolowski: I found it quite boring, but thanks :-) [06:58] can I get a handle to testing.T from inside a Suite method? [06:59] I want to call t.Run() to run some table-driven test function from inside my test [06:59] mardy: haha === zyga__ is now known as zyga [07:09] mardy: i had a quick look but don't see anything obcious [07:09] *obvious [07:16] good morning o/ :) [07:17] mardy: the usual way we do this is to just use a regular loop [07:17] mardy: and use Contextf that identifies the iteration [07:17] mardy: and pass it as the optional last argument to Check or Assert [07:17] I didn't look at how testing.T works but I know how the check logic runs [07:18] if you want modern subtest it would probably require changes to check [07:20] hey zyga [07:21] hey :-) [07:22] zyga: thanks! [07:22] yeah fwtw we didn't use testing.T before that way. loops are sometimes annoying when you need a clean slate before each iteration. in such cases we use a lowercased helper test function and multiple uppercase function for actual cases, calling the lowercase one [07:23] oh, yeah, that's another useful pattern [07:24] pstolowski: but the lowercase function, is it called directly, or via t.Run()? [07:24] speaking of testing, does anyone know why testing.AllocsPerRun returns float64 even though it promises the value is always integral? [07:24] mardy, directly from an upper case one [07:24] zyga: did you make a typo there? grepping for Contextf doesn't return any relevant match [07:25] mardy, func (s *fooSuite) TestFooWithSometVariant(c *C) { s.testFoo(123) } [07:25] mardy, check.v1.Contextf [07:25] oj [07:25] sorry [07:25] Commentf [07:25] my bad [07:25] np :-) [07:25] too much context lately :D [07:26] mardy, the TestFoo -> testFoo thing gives you SetUpTest and TearDownTest around it [07:29] zyga: thanks; in my case the test is quite trivial (operating on strings), so luckily no setup is needed [07:33] mardy: in that case use the loop pattern, see e.g. ctlcmd/get_test.go [07:46] * pstolowski physio [07:51] heh, matrix app scrolling broke in ff? the channel log did not scroll despite new messages :/ [07:51] anyways, morning again [07:54] zyga maybe that float64 is there due to stdlib backward compat? [07:54] hmm [07:54] perhaps [07:54] yeah, that's probably true, the original did not return integral values [07:58] uh... can you spot the difference? https://paste.ubuntu.com/p/BQ3CzkQv6F/ [07:58] or errors cannot be compared like that? [08:05] mardy, looking [08:06] mardy, you cannot use Equals like that [08:06] it doesn't compare objects for deep equality [08:06] only for direct equality [08:06] and two pointers you give are different (apparently) [08:06] it would only return true if err == expectedErr [08:06] and in go that's pointer equality [08:06] you want ErrorMatches or DeepEquals [08:10] mardy: yeah, ErrorMatches is what you want, and it's also easier when reading the code as the error string is directly visible as the expect value [08:11] mardy: one tip, if there's parentheses int he error message make sure to escape them, and if the error message has quotation marks, you can use backticks `` when defining the string [08:19] zyga, mborzecki: ah! Thanks! :-) [08:34] mardy, just remember that ErrorMatches converts the right hand side argument to a regular expressin [08:34] some escaping may be required [08:35] yep, it is indeed :-) [08:45] zyga: it's always fun when you think it should match, but doesn't and you end up slowly building from `.*` [08:46] ofc just to find that you forgot some random escape bit, or had a typo [08:46] mborzecki, yeah [08:46] exactly [08:46] that's why I'm slowly abandonging that and moving to something that understands errors.{Is,As} [08:46] but that's a long journey [08:47] I'll write ErrorIs and ErrorAsEquals or something similar, so far I've been doing that manually [08:56] re [09:15] pedronis: hi, i've updated https://github.com/snapcore/snapd/pull/10511 === Mirv__ is now known as Mirv [11:13] wth. for the second time i'm adding single comments in a github review, and the PR gets my approval ?! [11:15] pstolowski: github doing funny things again? [11:15] it seems so. it first happened last week === pedronis_ is now known as pedronis [11:24] pstolowski: thx, I'll do reviews this afternoon. My morning was meetings and meeting prep [11:25] pedronis: sure [11:25] ty [12:39] pstolowski: I did a first/incomplete pass on https://github.com/snapcore/snapd/pull/10515, I think the main code is correct but a bit confusing, I made some comments/suggestions [12:39] thanks === rbasak_ is now known as rbasak === zyga_ is now known as zyga [14:09] morning folks [14:20] do I understand right, that the steps in order to obtain the plug connection information (I need to read the attributes) when processing a snapctl command are 1) getting the context 2) context.Get("attrs-task",...), 3) load the task and 4) call get("Plug",...) on the task? [14:26] mardy: no, that sounds about something else unrelated [14:27] mardy: what are you trying to do? do you need attributes of an already existing connection? [14:27] mardy: that is about snapctl get during an interface hook [14:27] mardy: yeah, sounds like you were looking at interface hooks internals [14:28] mardy: you probably want to look at is_connected.go, that's much closer to what you need, you need to find the connected plug for your snap for the mount-control interface [14:29] +1 [14:29] mardy: it might be that in the end we should add a helper somewhere for this, but is_connected.go is the right direction for this [14:40] pedronis: thanks, I'll have a look (indeed, I need to see the attributes of an already existing connection) [14:53] cachio: sorry I forgot to mention, but while you were disconnected in the SU I mentioned I was looking into the refresh-delta failures, the failures are only on old distros like centos 7 and amazon linux 2 [14:57] ijohnson[m], right [14:57] I also noticed that [14:58] I need to spend time on that asap [14:58] because it is blocking all the prs [14:58] pstolowski: I re-reviewed 7700 again [14:59] as well [14:59] cachio: let me know if you'd rather look at it, I didn't make any progress, my theory is that the delta generated by the store is different than it used to be, and can no longer be applied with the old version of xdelta3 on those distros [15:00] ijohnson[m], right, still weird because it was working until last week [15:00] so, perhaps something changed in the store last week [15:00] cachio: well the deltas were re-generated last week with a new base series that the store deploys on [15:01] pedronis: thanks [15:01] so the deltas themselves changed, but for ubuntu's for example the deltas still apply properly and work [15:01] afaik xdelta3 wasn't changed in centos 7 recently [15:01] ijohnson[m], ah, ok, in that case I'll talk to them now [15:01] thanks for the info [15:02] cachio: ok in this case I will leave this failure to you to debug [15:02] that's the theory at least, anyway it means we might have to detect xdelta3 version and ignored deltas if older than something [15:03] ijohnson[m], sure [15:03] i'll let you know in case I need some help [15:03] it's a bit unfortunate if there is not backwards compatibility with newer xdelta3 generated deltas used by older xdelta3's but yeah as pedronis said it's just a theory [15:04] cachio: what I would recommend doing is get a shell on a centos7/al2 system, then look at the deltas downloaded by snapd and try manually calling xdelta3 to apply the delta and see what happens [16:12] * cachio lunch [20:44] ijohnson[m], hi, about https://github.com/snapcore/snapd/pull/10409#discussion_r668086150 [20:45] hey cachio what's up, was my comment unclear ? [20:45] I updated this because the test was not working using Cloud-init reported [20:45] it is super clear :) [20:46] initially we had a for iterating checking for "Cloud-init reported" [20:46] but it didn't appear [20:46] so after retry 1 minute the tests continue [20:46] when I updated to use retry command [20:46] I it started to fail [20:47] so I checked and saw the message is "cloud-init reported" instaead [20:47] so I updated that [20:47] ijohnson[m], does the change make sense? [20:49] cachio__: ah interesting so the message is indeed `cloud-init reported` you are right, so I wonder how the tests were passing previously ? [20:49] cachio__: but after you change it to `cloud-init reported` in lower case, does it still not appear sometimes? if that message doesn't appear, then the test should be marked as failed [20:49] so, on uc16 there is no messages at all [20:50] for uc18 and uc20 there is a message [20:50] hmm that's wrong, we should still see the message on uc16 too [20:50] before it passed because we were not checking at the end [20:50] cachio__: you see no message on uc16 with master ? [20:50] ijohnson[m], I tested uc16 and there was not message [20:51] I can remove the || true and we can see the error [20:51] to make sure [20:51] cachio__: ok, can you remove the || true in that case? I will start a spread run here so I can see what happens, which tests exactly ? [20:52] nested/manual/cloud-init-never-used-not-vuln/task.yaml [20:52] just that one? [20:52] yes, this is the only one ran in uc16 [20:53] cachio__: ok thanks I'll take a look [20:54] ijohnson[m], spread -debug google-nested:ubuntu-16.04-64:tests/nested/manual/cloud-init-never-used-not-vuln:refresh [20:54] In my history I see that one [20:56] cachio__: ack thanks I'm running it now [20:56] nice, I also :) [20:56] and also yeah I see why it was failing, we don't check that it was seen at exit of the loop [20:57] righht [21:12] ijohnson[m], my test already failed [21:12] tests.nested [21:13] ah I accidentally killed my test, I'll have to restart it [21:13] I can share my vm [21:13] cachio__: that would be great, can you PM the login ? [21:39] * cachio__ afk