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

Completely rewrite textbook module and its unit tests #198

Merged
merged 3 commits into from
Aug 18, 2024

Conversation

tianyizheng02
Copy link
Contributor

@tianyizheng02 tianyizheng02 commented Aug 17, 2024

Contributes to #45 by fixing all mypy errors in textbook.py as part of the module rewrite

As discussed in the CSC Discord server, textbook.py has been found to be broken. Parts of the underlying textbook API had changed, invalidating most of the existing code. For instance, one major change to the underlying API is that the API's request URLs now require CSRF tokens, which I believe weren't required before. Major breaking changes like this weren't caught earlier because textbook_test.py mostly didn't validate any function outputs, only their return types, and thus the module's unit tests merely gave the illusion of correctness.

I rewrote the module along with its unit tests to adjust for the changed textbook API, while taking the liberty to make the code more readable and robust based on discussions in the CSC Discord server. This includes better error handling, input validation, automatic retrieval of CSRF tokens, and much more comprehensive unit tests. The rewritten textbook.py passes mypy --strict --ignore-missing-imports with no errors and has 90+% code coverage with pytest.

In addition to the code changes, I'm also introducing several more sample/mock files in order to properly test the API requests. As part of the changes to the mocks, I replaced the data for the STAT department with data for the MATH department instead—for some reason STAT classes just don't seem to have their textbooks listed on the Pitt textbook site, meaning that there's virtually no textbook data available for those classes that we can use as mocks anyway.

As discussed in the CSC Discord server, textbook.py has been found to be
broken. Parts of the underlying textbook API had changed, invalidating
most of the existing code. For instance, one major change to the
underlying API is that the API's request URLs now require CSRF tokens,
which I believe weren't required before. Major breaking changes like
this weren't caught earlier because textbook_test.py mostly didn't
validate any function outputs, only their return types, and thus the
module's unit tests merely gave the illusion of correctness.

I rewrote the module along with its unit tests to adjust for the changed
textbook API, while taking the liberty to make the code more readable
and robust based on discussions in the CSC Discord server. This includes
better error handling, input validation, automatic retrieval of CSRF
tokens, and much more comprehensive unit tests.
@tianyizheng02 tianyizheng02 marked this pull request as ready for review August 17, 2024 09:46
@tianyizheng02
Copy link
Contributor Author

Future TODO: figure out how the Pitt textbook website computes the term ID for a given academic term. I have the ID for fall 2024 hard-coded into textbook.py and this is the best I can do for now, but we should definitely replace this with a function that computes this ID for any academic term. Otherwise, we'll need to change this hard-coded ID whenever a new semester draws near.

@tianyizheng02
Copy link
Contributor Author

@akrishan11 FYI, we should hold off on merging this PR until #197 is merged. This is because #197 fixes an issue with the grequests library, and textbook.py uses this exact library.

@tianyizheng02
Copy link
Contributor Author

tianyizheng02 commented Aug 17, 2024

@akrishan11 #197 has now been merged, and textbook now imports correctly without any MonkeyPatchWarnings or RecursionErrors. However, that warning/error can still occur if we import any requests-related packages before importing textbook, but we can't really avoid this.

@tianyizheng02 tianyizheng02 merged commit 6d6b1ef into dev Aug 18, 2024
4 checks passed
@tianyizheng02 tianyizheng02 deleted the rewrite-textbook branch August 18, 2024 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants