GitLab Code Review Weekly Workshop, 26 Aug 2022

Previous Meeting Next Meeting

⏯

youtube image

►

From YouTube: Code Review Weekly Workshop - Aug 26, 2022

Description

In this session we showcase some of the best outcomes from creating follow-up issues. We also discuss how to approach reviewing areas you are uncomfortable with.

A

Welcome to the code review weekly workshop, uh I had the first item on the agenda and I'll share my screen, so we're just all looking at the same thing, um and I want to share something that was like. This is a cool. This is like my favorite way that these threads play out, uh and so here was an mr, which was a community contribution, and I had made a suggestion, yannick you're familiar with this. We we worked in this together. uh We were like hey.

A

We really need to add some test coverage here, so I've suggested a patch of hey. Let's add this feature spec since we're adding a feature spec. We probably need to get this reviewed uh and peter uh left an awesome, non-blocking question of we do this a lot.

A

Maybe we should have a helper to do it and he created the issue and earlier I know we've alluded to the humongous backlog of issues, but look at look at how well scoped this issue is: uh uh there's a clear problem statement, the solution, a link to the discussion, and it was findable as um oh, I don't know if it had it looks like it doesn't, have a weight but, like I know, even if you add a weight to another labels to it um sure enough, pretty shortly after it was opened, um the community contributor wanted to work on it and they picked.

A

It up asked a question about just the approach and I got to respond with here's just the approach, but I was surprised then it was just like a couple of days later. This issue is closed, because community contributor created the mr got, you know activity on it and it got merged.

A

So I wanted to just encourage anybody listening that leaving non-blocking follow-up stuff, the best outcome, and it happens- and it's great when it does is um community contributors can pick them up and if we make the issues attractive to community contributors, um that's nothing but good for the lab wider community and our code base keeping things clean. um This was awesome. So thanks thanks peter for for leaving this non-blocking comment, it's easy to feel like hey. We shouldn't leave non-blocking comments, but an open source project.

A

I was also thinking about the mac stuff uh uh that it's probably really in line with our values and really helps us in the long run, if we're just very transparent of all the comments that we have so yeah. This is a good example.

B

C

Do you think it's better when you think as a reviewer?

C

Oh, this could be done in a follow-up. Is it better to just create the follow-up issue or to like let the author do it, because I'm really not I'm always not sure.

A

Good question: that's a really good question. um I think it depends on where I'm at like. If I have time, I don't mind creating it, it shouldn't take me more than five-ish minutes to create it. The hardest part is like for me. The hardest part is finding these labels to apply to it so usually without thinking about it. I'll just apply whatever was on the mr, that's whatever it is uh so.

C

Yeah I like creating them, but sometimes I feel like well. I have created them before. Sometimes I leave it up to the author. Sometimes I feel like. Maybe they don't get created. If I leave it up to the author all the time because it is non-blocking, so there's no action really implied there.

A

C

But I don't like just leaving that, like discussion thread in there, I like really what uh peter did was really helpful, because those issues that issue would not probably have been picked up by anyone.

D

C

It had the two proposal and the problem kind of detailed at the top.

D

A

C

A

I would say that by default, if you're leaving a non-blocking comment that you think would be helpful to be picked up actually creating the follow-up issue. Yourself is probably the best thing to do by default, but if there is um some really special information, that's needed to actually create the issue. That's probably when I would.

A

I wouldn't want to waste my time um yeah on it.

C

I feel like if it's small enough oftentimes, I think if I don't what I've done in the past, if I don't have time to write up like more details above it I'll just assign it to myself and then come back later and write up a little bit more details about the issue about what the fix is. I don't know if that's a good thing or bad thing. I still have some issues that are assigned to myself.

C

I I have like 80.

A

Speeches myself, I am not I'm not getting down that backlog at all.

C

No well, some of them most of the time. I will go back some of the follow-up um issues that were created, the yeah. The problem is so complicated. The write-up is not it's not like a simple it wasn't it's. You know it was more like this code is bad and needs to be completely refactored like.

B

C

Versus oh here's we could do like a helper method. That's great! You know, that's easy.

A

Yeah, I um we do have a handful of tools and it's not like chad and yannick. Yes wanted to hop in on something. So what were you? What were we all thinking.

B

um I highly do agree with what I've just heard, and one thing that I um could add is: I also make a distinction about how generic a follow-up would be, and what I mean by that is um the one that we're currently looking at from peter. This is like purely about our code base.

B

This is about a helper function to introduce, so that would be something I'd be perfectly fine to um as a reviewer create the follow-up myself, where this kind of shifts it's when this is very, very close to to the product we'll be looking at like, for example, insecurity is like just one thing in the vulnerability tab and we just have to add another top or whatever it might be. That is kind of where I, where I usually shy away from and ask the author to do it. That's usually how I go.

B

Oh I proceed with this.

A

What do you think.

D

Of chad, so I I had this happen recently too, it was maybe probably almost two years ago when I was working on the capture stuff, and I noticed that a couple of the services there was a good like dozen lines that was duplicated between them.

D

They're almost identical- and I was like you know this is probably should be cleaned up.

D

uh You know pulled out if there's a couple lines, it wouldn't be that bad, but a dozen is, was worth creating an issue, and then that weeks ago, a couple weeks ago, somebody came along and picked it up, but what they did was uh they were sort of in the, and I saw, coincidentally, an article right after this, they were in the peak of drop drought, page uh peak of drought, phase of their software development journey, and they like dried up everything.

D

They like pulled every single duplicate thing up to the parent class, and then they did some other questionable things like putting things in class variables, which is uh a really bad idea in rails, because you leak state between requests, and so I ended up. I spent a good. It was between half an hour and an hour spending a lot of time like helping educate them. I'm like you know.

D

This is really good, but I talked about coupling and cohesion and solid and how sometimes it's okay to just hold your nose about the duplication and that abstract you know, inheritance versus that and yes, we have to use inheritance and rails, but we have multiple inheritance. We can use modules like I did over here. Here's a great example- and maybe you just pick this one little part and try to do it as a concern and a mix in with the rails way.

D

I think iphone is coincidentally saw that article about the the peak of drought and sent it to them, and this is a giant uh comment I was like man. Are they gonna be upset? Are you gonna think I'm giving a hard time or just version or like no? This is great. I learned so much you're right, I'm probably in the peak of drought.

D

That was a great article, oh cool, but I want to do something on this I'll get back to it, but that was like two weeks ago and they haven't we'll see if they do.

A

Yeah and I think, with science follow-ups and if the intent is um the target audience for the stations, the community, the wider community, um you have to add to your like profitability bottom line of. Like am I doing this the most profitable way you got to add to it: training the community and.

D

I love talking about. Oh, oh, but there was this hospital like well, there's a there's an hour out of my day, yeah.

A

But it's good, I think it's profitable, it's good, and you know what you I appreciate you sharing that because it's so easy for us to be like. Oh man, someone's gonna, be apprehensive to us disagreeing with something they spend a lot of time on, but I think the wider community generally feels like they're honored guests in our code base and I think, they're very receptive to feedback. I I I think I have had maybe definitely less than 10. It feels like 5 or less actual pushback on suggestions for community contributors.

A

Usually it's just super smooth and they're really grateful for me taking the time and it's educating experience for everybody. um I appreciate you sharing that story. I think that's a good story like uh I can't get into you know I think we're getting better at our automated processes for seeing. If what happens when community contributions go stale so be curious to see what happens there, um but yeah two weeks in community contribution. Fine, that's just a couple of days really! You know.

D

Yeah and the the other thing is like, even even if it never, they never come back and finish it. It's like the the act of having to explain in detail like this is why you don't do this. These are the the software development. You know, fundamentals and philosophy behind this.

D

This is how you should do it a different way like that makes me a better programmer to have to articulate that in an extremely clear way to someone who may be less experienced because that's yeah, that's the only mark of a true expert is, if you can explain something complicated to somebody who doesn't understand it and that's really good practice, it's extremely time consuming, but it's fun and it's good practice. It makes you better yeah.

A

Well, I think it's a good litmus test of like if we're gonna the longevity of the code base. I think it's gonna be dependent on the culture and not just hyper focus on the code base, and so that's cool thanks for sharing that story. Chad, that was cool um leon thanks for the thanks for letting me share this example, so I thought that was. uh I thought that was interesting and helpful.

A

um Any other show and tyler questions before we hop into. um Are we using mariadb or mysql? What kind of databases are we using? I'm joking.

D

Equal light all the way light oh yeah.

A

C

Let me think if there's anything, that's come up recently for me, I'm trying to think of like reviews.

C

uh No, I mean, I guess, I'm always just struggling with like reviewing things that I don't feel comfortable 100 in or something that I don't do on my everyday, but that fits right into database reviews. I don't write database migrations. um Hardly ever I've done very few here.

A

C

At least by reviewing them, I feel like I'm keeping my skill set up to like what is required. What has changed?

C

um I don't know, I don't know if I'll ever be comfortable reading those uh explain plans, even though I've gone through the training multiple times, I'm still like, at least you know, I feel like I've gotten better at it before it. Just looked like some magical thing that I'm like okay, I don't know what that is, but um yeah, I'm.

D

Pretty still the same way like I haven't, I don't think I've written a single migration in my three years here, like I'm always off in some weird corner of the app or not even the main, app. That's not even rails. It's like not even you're.

A

Getting rails, man you're off the rails,.

D

I am I'm off the.

A

Rails, um no I'd love to check that out. uh Yeah, I'm pretty sure. uh What is it is it full outer join! That's the fastest one right, I'm joking! uh I I actually! I just know that that's not the fastest! I.

C

Would tell you I'd have to go. Look that picture up like every time.

A

D

Out what to do yeah.

C

I'm like okay, I need the little venn diagram. That's like what ones means. What, because I mean, I don't know.

A

Do you want to do you want to share your screen, terry? If you want to.

C

Okay, let me see if I will.

A

I can do it, I can do it just now. I can because I don't know, I don't know what I need to be looking at. So I don't.

C

Know either, but I will I can let me get my I can try to get my screen up and running hold on.

A

No, no, I got it, I got it, I'm already ready. So.

C

I'll put some links in here because I already shared uh the mr. I did not review this for a database. I think I did the maintainer for back end review so um and just because I mean I still do look at those files when I'm doing a back end review because I feel, like I don't know like the back end- is really the database as well. It's like there's not like some line that delineates it where it's like.

C

No, you don't look at that yeah, so I feel like it's kind of nice, because that means, if you do database, mr, you probably have like four sets of like a set of four people. Looking at it.

A

D

And being full stacked like it's hard for me to to not look at the front end code like when I call for a back interview too.

A

I I think this is touching on so like database. I think just bringing that up. You bring up a good point of like this can automatically some reviewers are like this is outside my expertise. I don't need to look at this. I don't want to look at this, but I think you bring up a really good point of like no. This is software and everything's connected uh and yeah. You could spend all the time reviewing just the back end, but if there's actually something fundamentally wrong with the database, yeah.

D

A

Going to affect the approach that the back end takes and it's going to probably affect the whole state model of the thing, and so I think it's wise to glance at the whole stack of what is going on. If I'm a front-end reviewer, I'm not worried about the details of the back. I'm not looking for like the human lending that we kind of.

C

Yeah, I would say, like it's hard for me, to look at the front end code. I have done view but again as soon as I'm done with it, I feel like it just is: like gets mashed into like the far corner.

A

C

Brain and never accessed again.

A

If you bring up a really good point- and I would almost say and correct me, let me know what you think: yannick and chad. I would almost say that front end review has more of a requirement to be aware what the backend is going on because of its position in the stacked architecture like front end is dependent on everything below it back end doesn't depend on the front end. You know what I mean like it should be aware of that light where it's consuming it, but it makes sense that bacon would be concerned about database.

A

But, like the concern of back into front end, I might be concerned of how it's consuming, maybe there's some things. I can make easier on the front end. Maybe but.

C

Yeah I have asked for an engineers before like. Can you let me know, there's a new thing, I'm exposing like does this ex, like does the way the data is presented, makes sense, yeah.

B

Are you having.

C

To go and do too much work to get everything the way you need like I mean, especially when you're, making something new um yeah.

A

B

Don't know, that's some, that's an excellent point. I do agree with what porter said that front end um the front end code kind of executes at the very last survey, kind of the yeah very latest in the chain. um What what you just mentioned is sometimes I wish that, especially on products that maybe switch teams or are in teams where people switch a lot or whatever it might be. Then we would have some proper documentation about the apis.

B

We are using between the front end and the and the end, and by that I don't mean a hammer file or whatsoever, so maybe something that we like. Okay, this is the interface we're connecting with this is this was added at this day for that reason whatsoever, because um yeah sometimes stuff is missing, sometimes front end is just jumping through all the firings um or things get past, which are actually not needed or even dangerous. So that is something that I feel like we can improve on are.

A

You are you talking about like distinguishing between if I'm writing hamill and I want to get a thing it could be from it could be really confusing. What's the single source of truth, for this thing, it could be.

C

A

Is there a presenter for this? I don't know is what is a presenter? I see model used everywhere. Control has variables, they use these controller variables like yeah. That's can be. That could be a challenge for sure, uh and I think we can run into issues when we don't use the single source of truth, because we we can end up circumventing permission checks um because we just directly access the model instead of going through something that actually that architecture is, is challenging from a front-end or perspective of what the backend does and yeah what we.

C

Think I guess that really um proves what both of you said. I mean you kind of yeah and sorry, friends and people.

C

D

C

Do need to know a little bit about the underpinnings, and I don't know how easy it is to go and say. Is there a presenter for this like object that I work with like I don't know, but it's definitely something you know. Maybe that's something I can take with me. If I get mrs that have front end and back end to look at that. That's, like the interaction level that I can look at in the front end code and be like okay.

C

How are they pulling the data from the back end in because when it comes to the view stuff it's like reading, I mean I don't know it feels like it's reading like some mix of xml and like french or something like two languages like I don't know, it's really hard for me to read.

A

It is like french, not at all.

C

No, it just feels like it's like two languages that it's like, I kind of understand, what's happening, but I don't know enough to uh be able to review it and say this is a bad practice or this could be improved, or um I think you showed some, mrs previously, where you were like. Oh the way, this um view uh object or this this method is being called. It's actually not correct, like I just don't have the domain knowledge.

A

Yeah, I think that that's okay, like and so like from a front-end perspective this. This is a great I'm going to try to abstract from this. As this effects code review in general.

C

A

I am going to be concerned from the front end perspective I am concerned about. um I do need to be concerned about the data I get in. I want to make sure it's correct the implementation details, I'm not going to be able to be I'm not going to be concerned about it's that interface. Like did. I actually get an authenticated value for this and that interface, I'm really really concerned about, and that's going to trickle a little bit to how we get to that interface, but the implementation details.

A

That is a little bit of a of a black box from the front-end perspective, and I I it's gonna, be a waste of my time to other front-enders time to feel like they have to know everything so from the database perspective, I'd say it's somewhere. It's like I'm concerned about the interface. Are there any like large, large issues, jumping out like big performance issues of like? Well, this just isn't going to work. We need to do something totally different or you know from the database perspective. The interface requirements are pretty much.

A

Is it normalized and if it's not normalized, there's got to be a freaking good reason and that's my understanding of of good rule of thumb for database interface uh and sometimes there.

D

Is like going all all the way down to the fourth normal form or whenever usually is not worth.

A

That's true, usually it's not the most performance, so it's like.

B

I don't know um quick quick saw before um before it goes away.

B

What I because you just mentioned the hammer interfaces once more and what I just said about the the missing documentation between the interface of the the backend and the view application thinking about it in the front and in 99 of the cases we don't actually test for it like it is super common to build a view, application which is require requiring some inputs, and then, basically, you know tests mock those inputs and after those tests are done, we can basically be like okay with all the inputs we're expecting.

B

This is actually doing good all good, so our frontend tests are more or less completely independent from what the backend is doing and we would kinda we would have all the flexibility or the room forever in the back end like we could leak anything into the dom um front. End would definitely frontal tests and yeah. We would not find it there. I don't know about the backend test suite, but that is something I'm kind of fearful of.

A

And this is a great point and I feel like it's.

D

And that's why they changed recently to have back-end reviewers required to review haml changes. It used to be just front-end, and now it doesn't happen as well, which.

B

Sounds like a good idea.

A

I and I think the thing to abstract there is that we all we all need to take ownership of the whole feature and make sure things get appropriately reviewed and if I feel like, I have no idea what ruby is. I should excuse myself from reviewing the hammel, because I can't just assume model dot.

A

You know I can just expose model.access token or whatever I mean I need, and and so things can get things I think it is worth all of our viewers to not get so focused on their expertise like, and we do have to all be aware of the the full stack and the whole feature. That's going on um it's a challenge. I don't know.

D

And the sanitization, for example, can be pretty tricky like there was one recently that I was involved in and I was like well, the doc said: don't use html safe. I thought that meant not everywhere, not anywhere and they're like well, it's okay, and actually this value has been previously.

D

You know, even though it's originally user input- it's been, uh you know, meant safe prior to this point, so it's okay, you don't have to make it safe in the view, but there's there's a lot of subtlety and you have to know like deeper in the stack, especially around security and sanitization of hamel. What's going on.

A

Yeah yeah and maybe that's maybe that's a takeaway as yeah. We all have our expertises. That's not a word, our expertise. Expert, I'm pretty sure it's expert expertise, um the, uh but it is all our responsibility to be aware of. What's going on and um a lot will fall through the cracks if we just focus on our own domains. You know if we, if we just focus on and that's I'm not sure how we can officially.

A

Support this other than trying to just really create a culture of short toes and a culture that almost has inverted toes that encourages the stepping on yeah; inverted those that encourages stepping on them.

D

Yeah, that's so gross and that I think that was what it was, at least at the crux of my discussion around the the maintainership and some of the lack of psychological safety of you know for required. Maintainership because, like I said I've, I don't think I've ever written the migration at gitlab, and you know, there's tons of things I haven't seen and even though I've done a lot of viewing a lot of front end like there's so many subtle things there that I just don't have internalized.

D

That's why I feel like I'm, just not competent to be a last time last line reviewer on the front end or the back end. To be honest, just because of the experience I've had and with the technologies and with the gitlab code base is not necessarily traditional.

B

C

B

um Okay, yeah hundred percent- um I don't know who needs to hear this, or maybe it's just for the recording.

B

But I highly agree with what chapter said like um our code base is way too big to know every corner or to know all the challenges about it and from my personal experiences, um I walk away from revenues quite a bit because I just don't know and I've personally never had any bad experience from it bonus points and whenever I find the time to I do this, I try to still follow it and if it's not completely off the hooks, I kind of look at the solution and see what's up and try to learn from it.

B

That really depends on how big and how complex this thing was. But um this is absolutely highly encouraged. In my opinion,.

A

Yeah, uh can I drop some thoughts to to you all on pertaining to this. I feel like.

A

No reviewer needs to know everything and I almost feel like no reviewer really needs to know anything except this one thing, which is how to ask good questions uh and everyone's at different level there.

A

The the only difference, then, between a reviewer and a maintainer from that perspective is maintainers, are really good at asking the right questions and they can spend much. They do it efficiently because they've, you know, put in the x hundreds of hours in the code base uh and so going back to this specifically man database reviews. This is a technology.

A

I definitely don't feel comfortable with, but yeah the code base is so large, but there's common quality attributes. We can ask at every level. Are there security issues here? I don't see any security issues here.

A

That's might not be an issue, but maybe if I'm really worried about it, it's performance issues here like and okay, have we heard performance? What was the previous performance? Can I get any kind of? Can I ask this? If I don't see here, there's lots of interesting data on it looks like well looks good. A database expert has approved this like so I I have more confidence feeling feeling good about it.

A

But I think just asking the right questions is, is that's that's the role you don't have to be the database expert, but getting more comfortable, asking questions, and sometimes that means asking them some. I know they say, there's no dumb questions, but sometimes you ask a question. You feel dumb for asking. So uh sometimes that means ask doing trial and error with your questions and that's. Okay, like we gotta have a culture where that's really fine to hold up an mr, because someone has questions. Let's, let's help our culture in that way.

A

I know what you all think of that.

C

um I, like I, I really did try to take something from our meeting last week about asking questions and um I practiced it and it was a good I felt like it was nice. I asked a question in an mr and the person linked me back to kind of something that helped me get more context in what I was reviewing, which was really awesome and I was like. Oh, it does work, that's good.

C

So thank you. I just wanted I'm trying to link it into the document, because I thought it was.

A

C

It was just a small question that I asked that I was uh yesterday.

A

That's something you should have shared on the show-and-tell.

C

A

C

I just thought of it now I, like literally sorry yeah. I was woken up at four o'clock this morning, bye baby, so.

A

C

But I'm sharing it now and then I'll be on the recording, so yeah. I think, and you almost feel silly sometimes I'll feel silly like about asking questions, and I do that. I'm like oh man, this is like, but in person I never feel them asking questions. I literally don't care. I don't know why. I feel differently, sometimes uh asynchronously, because in person I just don't I don't know, I don't really care what people think most of the time when I'm in groups um online, it just seems different and more permanent.

B

What I'd sometimes do about stupid questions? Take this refrigerator sold. I don't know if this is a good practice, because it's a little feared rhythm, but when I encounter something in an mr, I just imagine myself like okay. This might be a stupid question to ask, but if there's actually an issue here that I am now too shy to ask- and this thing gets merged or I'll, be merging this, and this thing explodes then I'll be the one who just did not ask the stupid question, and that would be that feel even worse.

B

So therefore, let's get it ahead from now, um it might be, might be stupid. Okay, I can live with that, but I'm having a stupid one merge that feels even worse. So that's that's how I approach it.

A

So when it all goes down, yana can say well, I asked a question about it and there'll be something.

D

So there's a lot to be said for cya driven development.

C

D

A

Yeah, that's funny. That's funny stupid.

B

Question on this side, what's cia for, like the od institution, cia, okay,.

D

No! No! No! No! No, but that's.

A

Related to it, I think, cover your.

D

A

Yeah cover your assets, yeah, that's good,.

D

uh The other side of that, though, is like I've spent a lot of time like down in one of those corners like the gitlab, flavor, markdown specification and testing and like harness around that and snapshot testing and there's not another reviewer on the back end team.

D

Currently it's just me so it's it's a bit tedious like when I've had to just pick a random roulette, reviewer and they're like what is this like, and they asked us some of the same questions over and over, and I carefully and intensively explain the same thing that I've explained, maybe two or three times before so I tried a different approach recently. I just asked in the markdown channel for like the the half dozen people that have reviewed these before.

D

Like does anyone want to volunteer to review these these three that I've got queued up since you've at least seen this code before.

A

Yeah, that's a good point and.

D

It's not rails, it's like not even related to rails, it's just like green field, algorithms and yaml files. So people look at they're like what is this.

C

Would it be- um and I'm not I'm just saying like thinking of this as someone coming in, would it be worthwhile to put some like a quick developer's guide together for the area that you're well.

D

I've got one that's about 20 pages, it's like they're, going to link it in every emr. This explains everything and I try to link out to the relevant sections and the documentation of the file. But finn is like well that's almost too much. That makes more confused.

A

That ask was quick developer guide.

B

Yeah, it's split.

D

Up there's parts of it I can link to about the parts and I try to do that. Yeah.

C

D

C

To think about it the same way um for what we work on with elasticsearch. I often worry that people don't have enough context to give it like a thorough review.

A

Yeah I so I have an approach for this like if I'm offering an mr that I realize this is a little different than the rest. The average thing you'll run into the code base and it's been going on without you for a while, so we're you know. How do we get brand new comers to review this?

A

um I will leave a lot of annotations, like I'm almost anticipating. If a community contributor reviewed this they'd be able to every line of code would have in every function. This is why, if something's really large, if I can't do that, because I'm making a really large change, I gotta loop in someone that has context, uh but I I do like to know like okay, I realize this is something that's very context specific, but it's small.

A

In those cases I love looping in people outside of the domain and giving them. You know some sort of exposure, increasing the exposure to the domain heavy code.

D

Yeah and that's that's something- I've really tried to be intentional about on these, like do a self review first and at least like the questions that I've had before pre-answer them or like this is split up like I had to split up the mr somehow I'm trying to be nice to the reviewers, but this isn't even used yet. But it's going to be used in this, mr over here.

A

D

So, that's why I'm putting it in now just to split it up somehow.

A

Yeah, so I have a um I have a question to you all. um I think I'm going to stop sharing my screen because I don't think this is adding any we've diverged from the screen. The question to you all being.

A

I guess two there's two questions. One is: have you ever run into an mr and you're, like I don't even know what's going on or how to start this review? This is really challenging and when that's the case you can either. I usually do this. You know double down. I am going to understand what's going on here and I spend probably way too much time doing it or do you think it's healthy to just say I'm having a really hard time. Understanding this, I think, there's context. I'm not aware of.

A

Could you the contributor leave some comments explaining blocks of code and why they're there.

D

That's a great suggestion because often you're a brilliant engineer, paul you're, almost always capable of the former. Sometimes I'm not I'm like there's there's no way. I'm figuring this out, that's literally impossible for me unless somebody sits down and explains it to me. Well,.

A

I still haven't figured out your your markdown scripts, I'm joking, I'm giving you a hard time chat. I'll, give you a hard time. Chad, I'm sorry! It's because I haven't read the documentation, functional.

B

um I think what paul just suggested is a great idea um what I encounter from time to time kind of even worse, where it is not so much about the the coach like, let's imagine and mr coming in within my field of expertise, the front end looking through the changes, kind of makes sense like I would need to see this running, but okay, cool and now the final boss, it's gonna, be in geo or like there is something we just do have features available that are so ridiculously hard to reproduce that are just way out of this.

B

um These are super tough. um I like paul's, idea what I usually do or sometimes do when I'm not just getting away from it. I ask for a sync session, um especially when it is about reproduction and like how can we make this the most efficient like I need to. I basically need to see the car running help me out, let's, let's pair up on this, um that is what I did in such cases. Yes, which is last sentence on this, which which might not be ideal.

B

Maybe we should start recording this or take it async somewhere, because the second revenue, they'll, probably just lose the information which is not great so finding a way to to preserve. This um is something something to work on.

A

Yeah yeah, I have another question for you all. um I didn't want to enter if someone else had something to say, though um the other question is it's interesting. How some of these reviews do require some parts of the code base?

A

Some mrs required us a level of expertise that is or familiarity with something that's happened before and to me, that's like that's. Like a code smell, I feel like how come this isn't intuitive like what can we do to make it more intuitive? But I I also you know, despite all my optimism, I'm a little bit of a realist where I realized there's some parts you just gonna have to explain, and I would like to hear from you all what's the litmus test for you all of knowing okay, this just requires some expertise and training.

A

This is unintuitive and we're. You know we're creating complexities that maybe we should avoid, because we've now created a silo where no one can understand it, except the people involved in building it.

C

B

Quick litmus test would maybe be. Is it in the docs camera because the stuff we're speaking about it is complex and we can't start from scratch or um every single time, but um things in the docs still complex but easy to to reproduce and documented. Obviously, rather than this is our product, you know how it works. Could you please do a review?

B

That's well. Maybe I could, but what about the next person? So that's kind of what I'm aiming for.

C

I like that question. I don't know if I have a great answer.

C

I like to look at how easy I mean I don't know I feel like. If something is easy to test, then it's generally okay ish.

C

um I say that very humbly, because the code area that I work in I feel like is very difficult to test, um but um I think I've gotten like most of my team on board, with looking at, like a pretty large refactor, at least starting to discuss what, like a new architecture for the code, would look like because I'm like really dreading new people joining the team and I feel like it inhibits like.

C

I mean if your team's not seeing community contributors, maybe that's a smell because they.

A

C

Come up to speed and it really shouldn't be a barrier to cut like that's a barrier to contributing.

A

That's a fantastic point.

D

Yeah we'll give.

B

You a shout out: let's go.

D

B

And that would be a mean okr for a team to actually try to attract those and get the get the issues polished up to to be able to take be picked up. Yeah.

C

Yeah I mean it does take effort to get issues to the point where people can pick them up, but also, I feel like, especially in the elastic search space, the just some of the way that the code is architected or just the way the code is laid out like it's hard to test and also as when I have to dig into it. I'm like remember how this works.

C

D

A

Good one, what we can say, chad,.

D

I was gonna say the that's a really good point, terry, that's what I was thinking like the coupling and cohesion right. It should be ideally cohes small, cohesive modules that are loosely coupled and the tests. You know tests serve many purposes, um not just for regression testing and not just to drive your architecture, but they're also serve as a form of documentation, and you should be able to look at the test and say: okay, here's the here's, my external interface, here's the assertion that I'm making about that and get some insight into it.

D

It's hard to write that sort of a test. Having said that that the markdown specification stuff I've been working on like itself is a a snapshot, characterization goal the master type framework and I'm writing those types of tests to do it. So I would say the tests are like there's not a lot to them, but they cover everything thoroughly but they're. Not that easy to understand. So I don't. I honestly don't think I've achieved that very well there, but it's also not even user facing code.

D

It's like code, that's driving some tests for our user facing code. So maybe the dial is down a little bit.

A

Yeah, I I think in your case what you're that domain chat is like you're you're dealing at like a couple of notches higher level of abstraction than most we're already in software land. But it's like.

D

It's not like a controller and a view. It's yeah. I.

A

Know some something about it is like. Is it it? It does make sense, but it is unfamiliar. It's like very unfamiliar territory than most people are uh that review. The gitlab code base are doing.

D

A

We don't usually do that kind of automated or intentional, with that kind of that kind of the main goal there, which even that goal has a level of unfamiliarity which that um that's interesting.

D

And I keep trying to talk myself down. It's like is this even needed? Is it? Is it design right? Is it over complicated and I talked to like enrique on the continent he's like? No, this is going to be really useful if you keep doing what you're doing like.

A

Because what you're doing is going to add so much confidence to making changes, but that's what's tough, is it's like layers removed from the main thing we plan on changing, but what you're doing is going to add so much confidence to that? That's a big point and that's going back to what terry said is man, I'm tess means so much.

A

It's so easy just to ignore them, um but they really do mean so much uh to the actual stability and longevity and fun that it is to work on on a project. uh That's a good point and I think it's critical to community contributions is tests. They they don't want to run the whole app. They probably just want to run the tests and they should be able to.

C

Maybe that's another good topic for I'm going to work near the end of the time here, but maybe that's a good topic for this. It's like what do people look at when they're reviewing tests, hopefully.

B

C

Reviewing that, but um I mean I try my best to kind of review things and making sure that I mean I don't always say like. Is this thing testing everything it should be testing like? I do rely a lot on that little widget that colors the little side of the line has this been hit? I really love that thing, um but yeah I mean it's definitely something that I think about. I do like to look at the the specs that are written.

A

Yeah you bro, you brought up something that got me. um You brought up such a passionate subject for me. I'm gonna have to and it just came up a couple of times so I was like. Oh I gotta do.

C

This so it's real relevant and like actively happening.

A

We might uh maybe it's worth sir there's not emerged by. um Maybe it's worth closing on this too, uh because we're reaching time.

D

Like while you're finding that that my answer to your question, terry, is I like, if they're fast like, and we have some unique things like the let it be, and let it be all of that oh yeah, like every little bit, hurts like. If you ran all of our tests into end on the back end, it will probably take a month if they weren't running.

C

I remember when I first got here: I just tried to run our specs because they didn't quite understand like the breath of what that did- and I was just like talking to my my husband.

C

um The tests have been running for a long time and they're, not they're, not finishing. It's really weird. Oh.

A

C

Because I used to work in places where there weren't that many like I would come in and there would be no tests or like very little and I'm like. Okay, I'm going to get to work on this thing, I'm going to add some tests here and they would still run pretty fast.

D

And that's hijacked but like what I've done on the stuff, I'm working on it since it's outside rails, I've intentionally taken that opportunity to make it a little bit more complex. The rail stuff is like in this little sub process, but all of the other stuff is with fast test spec and all most of my tdd of the actual algorithm runs in like 0.2 seconds.

D

You know 200 milliseconds, so that's really nice, it's fast, but at a little bit of extra complexity, cost yeah and it seems like our code coverage, doesn't always detect when you cover it in a sub process, which is a whole other problem. I'm just now realizing.

A

It should it really should, because of those artifacts that get built from the spec thing, but maybe not, I don't know.

D

I mean I'm, I have to look at that's my task today. Anyway, go ahead, paul.

A

uh Now I want to share this, mr is um removing lines of production. Scripts, lots yeah, it's just removing lots of lines of production, scripts and changing graphql files, and so, when I looked at this- and I saw okay well we're removing the relevant part in our other spec, but there weren't any test updates to this, and I was like okay. Is this a pure?

A

So then the question is: is this a pure refactor, meaning that if I enter, if there's a pure refactor, then if changing something like this, you wouldn't expect to change any tests, because these are all just implementation details and that's actually kind of the goal is to make this a pure refactor.

A

But I did my favorite thing, which is, I don't know so, I'm going to just add a bug where I comment out. You know these blocks in our graphql and do any of the tests fail and sure enough.

A

None of the tests, none of our unit tests and none of our feature, specs, failed and uh just for simplicity's sake. I was like okay, we don't have any feature specs covering like this time. Log feature uh this specific bit of like deleting a time log. So let's, let's cover this at the feature level and then maybe we can figure out how to push it down um down the pyramid, but this was really.

A

uh I was close to merging this, but I I I I did block it because I was like okay, we're just we're making this refactoring, but I want to be really safe with that, so we should probably add tests to it.

D

Our cameraman was like just kind of missing some bricks at the top, like that, exactly exactly like.

A

D

Is that the one that's sort of weird at the top.

A

Yeah exactly um this one was also interesting. uh This was a uh talking about um being optimistic about issues, so I this is refactoring a feature spec to run under the context of a subgroup and a regular group which came that requirement came from this issue that it created. You know pre-covet. I was like. Oh man, this is an old issue. uh Things were so much simpler, then, uh and it came from there was like a prior. There was an s1 bug that had an mr change.

A

uh This was the bug fix.

A

But we didn't have any tests updating this, but because it was so high priority that we fixed this right away. I was like okay, I confirmed this works. Let's just create a follow-up to add test coverage so that we don't run into this again. So two years later, it gets picked up to add test coverage, which is which is great, but what I realized was like okay. So if we added the test coverage me, reverting this two-year-old bug fix would have caused a test failure.

A

uh So I left that comment here because I was like we're already. um We are improving the tests already so much so this is good, but I'm going to leave a non-blocking comment of hey. I tried to revert the fix. That was the cause of us identifying some test coverage, but everything passed. So I was like maybe there's some more coverage we need to write. I thought we just need to do this. I don't see anything as actually doing this and flory on observation.

A

She was like we're not even ever calling that function anymore, so we have so that so all of this of like identifying test coverage identified wow, we actually probably have large gaps, because it's all vx actions and we've done this huge graphql migration stuff is like. We actually probably have a large bit that we could remove here. uh That is, isn't even being called ever uh so it was cool.

A

I just wanted to bring up too, like yeah keep tests are so central to so much of code, cleanliness uh that it's worth you know being treating them as a you know, top priority and reviews.

A

Yeah, those are my stories thanks for letting me share my stories.

C

That was great discussion. I know we're at the end of time, so I will leave the database link for next time if we do want to review it and maybe try to prep more. If I see a more interesting one, I picked one I picked because it's also graphql, which is um somewhat of a struggle for me. I will not be here next week, though, so if we want to push it out another week, that's fine yeah.

A

That sounds great. If I can get drop some thoughts for you to be mulling over your head about it, I would love to see what questions were asked in a database review and how that changed, because I think we can abstract from there. It's so easy when you're not certain about something to feel like well, someone worked on it and it works. I can't touch it, but I'd love to see like hey.

A

We did this, but we actually had to really change it and what was the catalyst to needing to change it, and those would be so much so much.

C

I'm gonna have to look for an mr that was reviewed by someone else then well.

A

I don't know there are some really.

C

Incredible, mr, like uh database reviewers that when this person's, like usually um there's a few people when I know that they're gonna get the review after me, I'm like okay, I'm gonna. I do what um yannick you said you do. I follow it cause I'm like okay, I will learn something here.

A

C

And I usually do so I'll try to see if I can find one and maybe leave it for two weeks out. Oh cool appreciate.

A

It terry thanks everyone for having on adios.

C

Have a good weekend.