Python Code Review: Adding Pytest Tests to an Existing Python Web Scraper

Key Takeaways

hey Sonny thanks for sending me your code I've got your github page open already cloned the repo to my machine and I've already have it open here in my editor I really like that you have a readme file that just explains how to set up some of these dependencies in requirements txt because I know that it's a little bit challenging sometimes to get El XML work especially depending on a platform but I was actually able to get this to run and install all of the dependencies I didn't go with the step here where I think this compiles it from scratch but I'm using homebrew as my package manager and Mac OS and I was able to just go brew install lip XSLT and something else that I don't remember I'm just gonna send you that in in a comment so potentially that would be an easier way for people on Mac to get this to run but in any case that was really helpful I really like that you have this here I also like the fact that you have a proper license file which I think it's just good practice for any open source project your requirements txt looks good it looks like that's actually also the stuff that we're really using there all the all the dependencies are pinned to specific versions which is something that I think is really good because if you know if someone were to not do that then we could easily get into a situation where people install this at different times and they end up with completely different requirements which just blow up so it's great that that you have those yeah so what's interesting about your project is that's a little bit larger than the project that I've that I've the projects that are if you've reviewed before for this code review series so we're just gonna see you know how much I can cram in before it just turns into a super long video that no one's gonna watch alright I've got all the dependencies ready and I just want to make sure I can actually run this locally all right it's auction results top pie let's run this it's doing something and then it blows up with a Nexen air connection refused and we can see here that this is because I don't have an SMTP server set up locally that's gonna send email so with this stuff I think what you could do you know maybe have something like a special flag like - - dry run that you check in here and if that's set instead of actually trying to send an email we're just gonna print print out that message so that someone who wants to run this doesn't have to actually set up an email server to send emails and I think something like that is eventually also going to be helpful when we're getting into writing some tests for this as you mentioned in the email you wrote me that you want to start testing this codebase writing some unit tests for it which I think would be a great next step and eventually you know we're gonna need something like that or we either either stub out or mock out that email sending thing one more word of warning this is probably something you want to look into so the SMTP lip dot SMTP it doesn't use an encrypted connection by default and I know there is I think an SMTP underscore SSL or I don't know how you know if it's uppercase or lowercase but essentially that one the other one is encrypted this one isn't so this might be okay for just connecting to localhost but could be something to look into if you ever want to run this and talk to a remote email server definitely look into the the encrypted connection here okay what I like is that using this pattern here to kind of factor out a main function instead of just throwing it into a script but kind of making sure that this is separated out and I really like that just you know one thing that's that's really jumping out at me right now is that this main function has these different faces here right like we're loading the config file we're initializing a bunch of settings and then we have this for loop and we do a bunch of stuff here including the email sending send results where where I would be thinking what if you would actually factor this out like pull it out in two separate functions and then you would end up with you know something like this where where you could say okay we have a function to load the config then we have a function for scraping the results you know on some specific site or whatever like this is not actually real code is more pseudocode but what would happen is that instead of having this relatively long main function you can actually pull out these parts and then that will give you the power to test them separately so maybe to demonstrate that I actually want to do this refactoring with the load config function because I I think that could have the benefit for you also where in the future you know you're probably gonna need something like that repeatedly right like eventually you're gonna work on another project it's gonna need some kind of config file parsing and if you already settled on yanil for that which i think it's a good choice then you're probably gonna want to reuse something like that so I think it pays in the long run to actually write these little functions so that you almost have your own library of things you can reuse and all of these bricks you can use reuse to build new programs so yeah so like I said you know we're gonna refactor this thing a bit and then see how that works out so I'm just pulling this out into a different file here and now I think what's interesting is that um it gives or that that piece of code it actually has the config file hard-coded I would probably change that so that it takes a filename and then the next thing so okay so this is completely random because I actually worked with this you want to look into the yamo library and the load function because there's also a safe load that's probably gonna be the better choice in this case so kind of the the problem with Yama load is that it's it's relatively insecure so this is similar to the the pic whole module in the standard library which basically has the power to execute arbitrary Python code that's fine if you completely control the the input here let's say the config file you know if that's always just loaded locally you write it you know you're a hundred percent sure there's no malicious contents in there then using dot load it's totally fine however if this piece of code at some point ends up you know in another project gets copied and pasted around I've actually seen this happen at a client where I worked where there was a very insecure Yambol dot load in production with user data and luckily no one exploited it but it could have been you know very very easy to do that so yeah my recommendation would here would be definitely use the safe load even if it's not giving you a huge security benefit right now but it's just gonna keep you a lot safer in the future ok so now this parses out the config file right so because now this is a function I would just say alright it's gonna return a the actual config dictionary right and in here we could just say config equals load config and I believe it was config dot yam all right and then I can actually drop this comment here well a because we've just completed it and B now I don't need a comment that says load the config because the function is called load the config right so I'm giving more explicit names to everything which personally I think that's a good thing so the next thing here is I first of all I think it's great that you're actually checking for errors here I think that's good what I would try and do is to actually give a little bit more output here with the you know what would actually went wrong so when I'm going config Yambol and we're just gonna move that to rename that to config back right so now that that file that we want to load is gone so if I try and run that program again it's gonna say fail to read config yeah mo but we don't know why right I could have also edited the config file and introduced some kind of syntax error and then I would get the same error so what you can do here is you can just hold on to that exception and then do something like this where we're just gonna tack on the exception message and now if I trigger the same run I actually get this this error message here and that tells me no such file or directory config yamo which i think is a little bit more helpful so let's undo that sure it still works yeah so I would look into that okay so now the next challenge is that because here we're actually we've turned this into a function I'm not sure if this should have the power to exit the whole program by itself right because if you imagine you want it to test this load config function a challenge with that would be that you don't want to exit the program all the time and there are ways to prevent that from happening but then you you know you really have to kind of dig around in the innards of Python and kind of monkey patch some stuff to to prevent that from happening which is in my opinion not the best way to go about it so I think what this function should do is we want to maybe define a new exception class so we can just say invalid config it's just based on exception and that class doesn't even have to do anything where you but what what we're gonna do now is instead of triggering an exit we're just gonna say okay throw this error with the message we were using before and then you know it's already pretty clear what's happening like I don't have to comment this heavily I don't have to pass around some kind of string it's just yeah I get an exception and I know what happened so okay so I actually kind of got a do like break that config again and then run this again all right see so now already this is getting more and more explicit right and now we're getting this invalid config failed to read config yeah Mel no such file or directory config yeah mo and actually just noticing that we've got the the name of the config file hard-coded here as well which now isn't even true anymore so I would probably actually just change this so that the error we get in the end is going to be invalid config no such file or directory config yeah mo and I think that is cleaner or actually clearer to see what's going on now the thing is like that actually changed the behavior of your program in my opinion would actually be fine to do something like this where you just let it die with a with an exception and then you get the full stack trace and we know what happened I think it's very it's verbal enough to for people to figure out what's going on but to actually make sure it behaves the same way as you had it before I would do this invalid config and then we could still say exit like we will probably want to print it and do an exit one so now we'd get exactly the same behavior we would lose the stack trace so you know arguably that isn't the greatest the greatest results here we probably want to make this a little bit more explicit so something like that I think emulates fairly closely what you had had earlier and then here instead of doing the exit again I think main shouldn't really have the power to trigger an exit by itself but what it should do and you know this is a minor point I just like to do that what if you just returned the error code you know then it becomes kind of a very C like and where you're returning the actual error code and at the end we're just gonna call exit on the the return code here okay so let's copy that back make sure everything still works at the end I'm not actually sure what happens when you call exit none so just to make sure we're probably gonna need to return a zero just to make sure this is still behaves the same but then so that why the reason why I made this change is then if you're gonna write test for this eventually you can actually test this whole main function and it's not gonna exit the program it's just gonna give you a return code that you can then test for in a test case or check for in a test case and that will give you the power really to to also reuse this in in different different programs and in different scenarios and so I think this is a change for the better so I'm not gonna do you know similar things with this other stuff I think will be really easy for the the email sending part but I think the general technique that I've applied you could use that and use it in other parts of the code as well right like here for example we're loading a sample file again you know this would be an opportunity for you to turn that into a constant maybe use it in a separate function and so on so I think that would clean it up a little bit I mean there's you know this is super nitpicky this is really Pep 8 because the lines are too long but you know maybe you're using a different setting like the one reason why I like keeping my lines at 79 characters or kind of the peph eight recommendations is that in github code reviews it's just so much easier to have two files side by side without having to scroll horizontally and I feel like it just makes it so much easier if you're working with a team so much easier to review each other's code if if there's just no opportunity for you for people to scroll horizontally because it's just it makes it so much harder to line them line up the files and actually get an understanding of what's going on at the same time but yeah like I said this is super nitpicky and in general I think your code is formatted formatted well you know it's it uses consistent formatting uses consistent spaces for everything and I really like that okay so I'm gonna try and like speed this up a little bit because I don't have too much more time so here with this thing initialized global settings mm-hmm I'm I mean I understand why you're doing it but I think you would be better off actually rewriting this at some point so that you're not just reaching in into this auction suburb suburb class and setting these things but instead you're passing it in every time you create a suburb object I believe right so right now what happens like we're reaching and actually changing this this attribute here and I would try and change that so that you're actually passing this in and as a a different argument to the dunder init method because then again again it becomes easier to test this piece of code right where now you would have to make sure that every single test overrides the settings the changes that the previous tests made and this all this housekeeping just makes a lot harder to write tests but if you changed it so that you would just create a suburb object and every single time you just pass it in that base URL you know which isn't all that much work then you can be sure that every suburb object is actually independent and doesn't have these global settings so I would probably change that okay yeah I think I mentioned that before you could do something similar here with the sample sample data I would probably also extract these guys here - just to be constants I'm not sure if they should be in line you know probably just have these as constants up there and then set them instead of having them as plain strings here I like the fact that you're writing comments and yeah like I said earlier you know if you if you refactor this a bit you could actually in my opinion get rid of these comments because the code is gonna speak for itself right the load config part here it's actually gonna tell us exactly what's going on [Music] okay my suburb yeah this is a bit of inconsistent formatting I guess I can Python you would always you know do this let's leave that for now like you're you're changing the the casing style but yeah that's that's not too horrible okay okay so let's actually look into the suburb module real quick again okay it looks pretty sensible so far I guess a couple of random things um you're switching quote styles which that's like another super nitpicky thing that I bring up all the time but I really like them to be you know consistent for yeah actually why I don't know I just like things to be consistent because it otherwise I my OCD kicks in and I just get this like okay why ice is forming it differently if I want to search for for all the strings in a file you know it becomes harder to write the right reg X to do that and all these things so yeah for some reason I'm just a stickler for making that consistent yeah okay so there are a couple of things and this is actually being called out by the linter as well so I'm seeing that you're you're doing this right where are you going if none equals equals separator and so I'm thinking you're probably doing it in this order to avoid like isn't this like some kind of C trick to avoid turning a comparison into an assignment like if you had if x equals equals null right and then we're doing something and then that could accidentally turn into x equals null and then you would this would always be true and it would be a huge bug essentially and if you're flipping this around null equals equals x then if the same mistake happens it's a compiler error because you can't assign to null so I I think that's where it comes from now the idea behind that is actually really good because the same thing could happen to you in Python right like you could be like separator equals equals none and then actually dentally do this and then you can end up with an assignment which you're not sure if this is this valid Python let's see okay yeah so you can't actually do that in Python but the idea is good right like you're you're thinking of stuff like that which it's something that I really like so actually the recommended recommendation Python would be to to not even use the equals equals comparison I mean this is being called out by the linter as well right so flake eight-tails mean comparison to none should be if condition is none so you want something like if separator is none and the reason for that is with equals equals you're checking for equality and that will depend on how that object or the object you're implementing how they are at the objects you comparing how they are implemented so you're you're not you know some of them might actually say hey yeah I'm equally none because I'm empty or whatever but you don't want that usually what you want is an is comparison which checks for identity so in this case none is like a global singleton there's there can only ever be one none object in an a Python process and therefore if you're using the is comparison you're on the safe side because then you know you also can't accidentally change that to an assignment and we just established that that wouldn't even work but this is the way you would compare to none and you wouldn't use the the equals equals comparison right and it would be the same here [Music] where you would just use is not none and this is interesting because again I think it really shows how powerful a good linter like flate if lake 8 is where it actually calls out these things right and tells you about them so i think again you would really benefit from from actually running flake 8 over your your code just to maybe demonstrate that really quickly how you would go about that just install flake eight my running pip install flake eight and then we could just run that on the auction module and we should get yeah forget a bunch of messages here where we're comparing to none and and this seems like it's happening quite a bit so you know maybe you want to look into that and actually just train yourself to use is to avoid these comparisons but overall it's looking pretty good I like that you're breaking up these yep that would work I mean again here you know I would change the quote style on all of them just to make them consistent but I'm you know I'm not going to be trying not to be so nitpicky this is a bit weird so we're creating this feature dictionary and then setting it to the empty dictionary not entirely sure why we would do that yeah I think I've seen that a couple of times were you computing the hash of something again that could be its own function I think it would be totally fine if that was its own function right it's up here as well we're doing the sha-1 I mean here you're passing an image but with all of this stuff maybe it makes sense to actually extract this because then it makes it really easy to change the hash function globally for example mm-hmm yeah okay so I think my general comment here would be that maybe again this is doing this object is doing a bit much like so my main feedback essentially here would be that this seems to be doing some parsing and scraping right we're running a beautiful soup on this thing now [Music] what this also does is is actually it creates these emails that we're going to send and again I think you could come up with a structure where these are separate concerns right so you have like a supper parser object that is only concerned with parsing out some HTML that you hand it and then you would have a suburb massive suburb report generator or something like that that could take a parsed suburb and then based on that actually generate that report or maybe it would take multiple suburbs like essentially you could split up these two concerns of parsing the HTML and generating the report I think I would probably split those apart again that's gonna help you with testing we're gonna get to that in a minute and I think it would make your code a little bit cleaner and then also you know these classes would be if you split that in two they would be half as big which I think would be good in terms of maintainability and like I said for writing tests and just understanding what's going on so I think this would be helpful I don't want to go too deeply into this but what I want to do is I want to get a little bit into testing just because you mentioned it and so we're gonna do that in a minute all right so one more thing I wanted to dig into is just give you you know a very simple set up for running tests so you can get started in writing tests because that's something you actually mentioned in your email so I like using PI tests which is kind of um yeah it's a Python testing tool it's kind of an alternative to just using the built-in unit test way of structuring your tests it has it requires a little less boilerplate code which i think is good I really like simplicity and it is also a very good test runner that will find your tests automatically so you don't really have to worry about running them manually writing scripts to do that so I'm a huge fan of Pi tests and I just want to show you how to set it you know how to set it up in how it all works so all we need to do really is we need to pip install PI test and what I want to do is I want to write a little test for this load config file with the refactoring we did earlier so what I would probably do is just create a new file that we're gonna call test config let's call this what should we call the module config parsing PI I just want to make sure this actually works so what I would usually do first is have a failing test so I can make sure we're actually running the tests and then once I have something that fails I can add to that and kind of work towards where we have a test a suite of tests that actually tests something so what I need to do here and you might not have to do this is I need to ignore my virtual environment so that PI test only finds the test is actually relevant to us all right so now to run this test just gonna go PI test - - ignore equals vnf because I need to tell it to ignore the virtual environment folder because otherwise it's just gonna find a crazy amount of files to test that are really irrelevant to our project here your project here okay so this is running it found the test config parsing and it actually the test failed right just like we wanted it so now we know this works so in order to test this I think this could actually be its own module so what I would probably do here now is create a new module we're just gonna call config parsing PI I'm just gonna dump all of that in here and you can get rid of the Yambol stuff I think as well then I would say from config parsing import load config and invalid config using gamble anywhere now okay so now let's make sure your program still works which is you know if we had a proper test suite then we would just run tests and hopefully that would give us a good idea of everything's still working okay all right so now in the test config parsing we're essentially going to do the same right we're just gonna import that and now we can start writing our tests so I think a good test would be to to try and parse a simple Yama file well actually let's try and make sure if we give it a file that doesn't exist it's not gonna work so test missing config and you always want to start your so essentially how bad the PI test runner works is that it loads all the modules let's start with test underscore and then finds all the functions that sort with test underscore in those modules and then runs them as test cases and the way your your testing conditions in tests you just use the built in assert statement and then PI test is essentially going to do some magic to give you really nice error messages for these tests so in this case I would just say load config does not exist yamo right hopefully that doesn't exist so now the thing is with that is because that function now raises an exception and invalid config exception we got to make sure that [Music] this exception we expect this exception to come out right so there's actually a helper in PI test to do that where we can say with PI test races invalid config and then it that's our test case so when we run this it passes and what this helper statement makes make sure is that actually you know we throw like this load config function actually throws a invalid config exception so if I change that to just return 42 we should get an error that we're not calling an exception right did not raise invalid config that way we can be sure we're actually testing the right thing all right so the next test I want to write is one where we actually try and parse out a valid config file so we're just gonna call this test valid config don't keep it there simple so now the challenge with that is when you look at this load config module it actually uses open to parse out a file from the file system and this stuff is always a little bit hard to test but I also really like PI tests because it has some support for that kind of situation with its fixtures mechanism and so the way this works is and this is gonna seem a bit magical which is some well I think that's a bit unfortunate but on the other hand I think it's really powerful so maybe just bear with me and see if that you know feels right to you I think it in this case would be a really good way to test this piece of code so what you need to do is just tell PI test to give you a temporary directory fixture and that just works by giving it a temp Deere argument and then magically PI test is gonna give you a temporary directory to work with that so we can just say okay the temp config is something that we create in this directory right we're just gonna create a new file here temporary file and then we can actually write stuff into this file write text so we can just say something right let's just say hello yes and then we also need to pass an encoding we're gonna go with utf-8 here and now what you can do is you can call ster path or you can take the star path off that config file or that temporary file and this is actually going to be the areal path that then open can work with and and read through the contents that we just wrote to that file so we can actually say parsed can Figg equals load config give that path and then we can do a bunch of assertions right we could say parsed config hello should be equal to true and it actually worked right because the animal thing is gonna convert the yes to true and you know just to make sure this actually is making sense let's actually get this to fail so that's one thing I always try to do break the test before you're moving on because otherwise you're not gonna know if it's really actually testing the thing that you want to test yeah so in this case we're getting an assertion failure here and I think this looks pretty good I mean we could go we could go crazy with this and you know make this more complicated which maybe is worth it it's a good question you know like we could we could actually change the contents here make that a little bit more complicated right like if we wanted to do something like this we could do that oh wait like this I could say okay hello yes on number [Music] 42 and then we're gonna write that out to file and then we could say okay hello is true yeah let's just you know make sure it actually fails but kids - oh yeah never mind maybe - don't need that okay so we could run this okay and then we can have a little bit more complicated tests so yeah so this should still work okay so this is looking pretty happy and you can pass - V for verbose mode and then you can actually see a little bit more closely what's going on here okay and so now that we have our test for a valid config file I would actually do the same thing but for an invalid file and let's move that up here for super consistency so all the bad tests or in one section and the good tests are below that so I'm here let's try and come up with a way to actually have an invalid yamo file I don't know let's just put a bunch of percentage characters here and now what I would expect to happen there is this to actually throw an exception so let's just run this okay and we're getting a crazy yeah mol scanner error here and there is actually I think there's like a base class for all of the yam Belarus right which is called yeah no error so I think what we want to do because we actually found a problem with load config here as well where right now this is just looking at OS error and IO error that this should also handle other yama lairs that we weren't really handling before so now when I run this again it's throwing an invalid config exception and then PI test races I can do the exact same pattern here there we go and oh wait what did I screw up now oh okay I'm missing closing braces okay so now we're again we're checking for an invalid config exception yep doing this so now I think this is looking pretty good this is not you know by any means exhaustive yet but I think for a program program or a tool like that like we have here it's actually pretty good test suite you know just for this little function yeah I guess probably what what you're thinking I was like oh man this is gonna take a lot of work to write all these tests yes on the other hand they're also gonna uncover a lot of edge cases and stuff that you know like going into this I didn't realize like we're actually missing that that piece here where we're checking for Yama lair and this might not actually be you know it might not make sense to spend that much time if it's just a simple script or as a small program that like you have here but just having the ability to write these tests and then kind of building on top building on top of them I think makes this whole development process a lot more I'm sustainable in the sense that you know you can eventually use that config parsing module in another project and if you just bring over that whole test suite you're you're gonna have a pretty good idea of well you know you're gonna have gonna be able to put a lot of trust into it and so I think that pays dividends but you know in the end it's it's totally up to you if you want to go down that route do check out PI test I'm a huge fan of that and admittedly this was a little bit more challenging to test because we were actually access accessing a file here so you know normally you wouldn't have to have to deal with all this craziness but we would kind of focus on simple assert statements just to make sure your your functions work the way you want them to and you know if you want to go deeper into this I would actually recommend that you try and do similar things with the rest of your code right like try and break it out into simple sort of self-contained chunks and then write tests for those and I think whatever happens there this is gonna be something that is gonna help you in your in your Python journey and it's just gonna help you establish these these patterns and you're gonna see stuff like that sooner then right because you're you're gonna be writing these tests and as you're writing a new function you're gonna be like oh how do I actually actually test this and this is gonna create this like feedback cycle where the need for writing good tests is actually gonna influence how you write your code and usually like I believe that code that is easy to test is usually also easier to understand and easier to maintain so yeah look into that it actually seems like a really cool project seems like a really useful thing that you've built here for for yourself and it's a great sort of baseline or great result you can build on top and it's just kind of you know make it nicer and cleaner and nicer maybe just as an exercise to try and write the cleanest Python you can like I find that a lot of fun so maybe you'll enjoy that too cool that was a bit of a long one so I hope this is gonna be helpful enjoy the video and talk to you soon