unc-csxl / csxl.unc.edu

CS Experience Labs' web application.
https://csxl.unc.edu
MIT License
10 stars 11 forks source link

Refactor All Tests to use Core Data, Improve Testing Documentation #406

Closed ajaygandecha closed 4 months ago

ajaygandecha commented 5 months ago

This pull request modifies all of the existing pytests to use the core_data.py file, which provides a single fixture for all test data for the database to load at once. Previously, some files used core_data's fixture, while others manually imported fixtures directly. This manual importing often caused confusion, especially for students in 423! Fully embracing the core_data model will improve code readability and make testing easier to set up.

This PR also adds to the testing docs page to provide an explanation of how fixtures work. This new documentation might also be good to send out to students for Sprint 02.

ajaygandecha commented 5 months ago

Also, future consideration: We could refactor further to make a simple setup.py which includes the conftest and core_data fixtures in one, so test files only have to include this one import line.

KrisJordan commented 4 months ago

I like the effort to simplify testing files, however this approach has one significant drawback to the current approach which I believe we should address as the site continues to scale with new features: tests are able to load only the data they need. As these tests are more integration-like than unit-like, because we are testing integration with postgres and all data makes its way to postgres, there is a cost to loading data beyond what is needed for a test. It seems to me within a given feature (academics, office hours, coworking) there are known data fixtures which we could boil down to a single file within the given feature and achieve effectively a very similar outcome while not loading data unrelated to a feature unnecessarily for each test. Thoughts?

ajaygandecha commented 4 months ago

@KrisJordan I agree! After revisiting this, especially with the office hours feature, I think I will close this PR. I will open a ticket to refactor test data to how you described above, I think that makes more sense!