Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update test files for boxscore module #500

Merged
merged 1 commit into from
Jan 5, 2021

Conversation

thomaslu2
Copy link
Contributor

What I'll be doing in the future is create a PR for each module of test files.
I did come across some issues when modifying these files, as seen in issues #455, #498, and #499.

Let me know if you want me to go back and change something in these files!

@roclark roclark added hacktoberfest-accepted Recommended issues or pull requests for Hacktoberfest quality of life Any updates that improve the overall quality of the code labels Oct 16, 2020
@roclark
Copy link
Owner

roclark commented Oct 16, 2020

Hey @thomaslu2, thanks for putting this together! Looking through the updates, I don't see anything that really stands out, but I wonder if it would be good to remove the old sample HTML files alongside adding the new ones just to keep the repository clean? Otherwise, I believe all of the issues you identified have now been resolved as of the latest updates, so hopefully a rebase will clear this up and we can get this merged! Thank you again for this, it will be very helpful to have recent stats with the latest updates, and it helps that you are flagging the issues you've run into as well! 😃

@thomaslu2
Copy link
Contributor Author

Yeah I can remove the older html files for you. This PR is good to go once I fix the merge conflicts and whatnot. I'm still working on the test files for the other modules so keep your eyes out for that.

@thomaslu2
Copy link
Contributor Author

This should be good to go now

@roclark
Copy link
Owner

roclark commented Nov 13, 2020

Hey @thomaslu2, once again, sorry for the delay here, it's been a crazy month year. The updates themselves look good from a content standpoint, but looks like there are some stylistic tests that are failing with pycodestyle. If you can resolve those, I'd be very happy to accept and merge this! Once again, thanks for the patience with this. I greatly appreciate the update! Let me know if you have any questions on the updates.

@codecov
Copy link

codecov bot commented Nov 13, 2020

Codecov Report

Merging #500 (1f6d4d0) into master (cb97edc) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #500   +/-   ##
=======================================
  Coverage   99.97%   99.97%           
=======================================
  Files          55       55           
  Lines       13178    13178           
=======================================
  Hits        13175    13175           
  Misses          3        3           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb97edc...1f6d4d0. Read the comment docs.

@thomaslu2
Copy link
Contributor Author

Hey @roclark, this should be good to go now. I do see that the codecov reported a -0.04% on code coverage, do you want me to go back and try to recover those lines?

@roclark
Copy link
Owner

roclark commented Nov 14, 2020

Thanks @thomaslu2! If you wouldn't mind covering those lines, that would be great. It looks like the tests no longer cover the scenario where it can successfully parse the home team's record for some reason for both NBA and NCAAB, so the wins and losses default to zero. The easiest thing would probably be to create a unit test for each of these by mocking the self._home_record value to be valid and testing the boxscore.home_wins and boxscores.home_losses values. Let me know if you would like any help and I can provide some additional pointers. Thanks!

@thomaslu2
Copy link
Contributor Author

@roclark should be good to go!

@roclark
Copy link
Owner

roclark commented Nov 17, 2020

Hey @thomaslu2, thanks for the incredible work! Everything looks really great from a content standpoint. I hate to add on one more request, but would you be able to rebase your commits into either a single commit which includes all changes, or a commit for each module (NFL, NBA, MLB, etc.)? Ideally, the commits would just be for new updates from the upstream master branch and not fixes for code that is also a part of this branch. Once things are squashed down, I'd be happy to merge this! Thanks again!

@thomaslu2
Copy link
Contributor Author

@roclark I'm not too sure if I'm able to squash the commits since I had to pull in your changes from the master branch. This caused a merge commit to be created and it makes it harder to squash because of it.

@roclark
Copy link
Owner

roclark commented Dec 15, 2020

Apologies once again @thomaslu2 - I've been utterly slammed throughout the year, especially the second half. I agree that the squash method isn't the most elegant as-is, but I just ran through a trial on my side which worked well and I think offers a pretty clean solution. The basic flow I did was the following:

  1. Checkout the master branch and pull the latest code.
  2. Checkout the thomaslu2/update_test_files branch.
  3. Rebase the update_test_files branch against the master branch using git rebase master.
  4. Resolve any merge conflicts (mainly accept the incoming changes).
  5. Verify the files are as expected (ie. compare the local tests/integration/boxscore/test_ncaaf_boxscore.py file with the upstream version). I created a temporary commit to house any other updates that were required, but this can be done with the merge conflict resolution above.
  6. Rebase and fixup all changes down to a single commit (the "Modified mlb boxscore test" commit). I did a git rebase -i HEAD~10 on my end.
  7. Edit the commit message to indicate the updates for all modules.
  8. Force push the new branch.

This will reduce everything down to a single commit which will contain everything, and we won't have the merge commits in the updates as well.

Hope this works out. Again, I apologize for all of my delays, but really do appreciate the help here.

@thomaslu2
Copy link
Contributor Author

@roclark Sorry this is late! The holiday season was pretty hectic and I didn't get a chance to look at this until now. This should be good to go now, thanks again for the help! I will continue to update the test files for the rest of the modules.

@roclark
Copy link
Owner

roclark commented Jan 5, 2021

@thomaslu2, thank you so much!! And you are not late by any means - I'm the one who has been embarrassingly absent during the process. Thank you again for all of the great work here. This looks fantastic and I will go ahead and merge it. I really appreciate the update and putting up with my various requests. Hope you had a good new year!

@roclark roclark merged commit 98f15d7 into roclark:master Jan 5, 2021
@roclark roclark added this to the Release 0.6.0 milestone Jan 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Recommended issues or pull requests for Hacktoberfest quality of life Any updates that improve the overall quality of the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants