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

feat: removing visible_date attribute #2511

Merged

Conversation

deborahgu
Copy link
Member

removing the reliance on the visible_date attribute

  • re-factoring some tests, not just to make them work with the new code, but also because some of these tests were never ideal. They weren't all testing the right things, some of them never tested the code path that used certificate available date, and definitely a bunch have non-useful names and no comments. Several were testing impossible (or, definitionally broken) situations like program certificates with no corresponding course certificates.
  • added django stubs for type checking
  • added some type checking

FIXES: APER-3238

Run JavaScript tests locally with Karma

There is work being done on a fix to get Karma to run in CI. Until then, however, contributors are required to run these tests locally.

  • Make sure you are inside the devstack container
  • Run make test-karma
  • All tests pass

removing the reliance on the visible_date attribute

* re-factoring some tests, not just to make them work with the new code,
  but also because some of these tests were never ideal. They weren't
  all testing the right things, some of them never tested the code path
  that used certificate available date, and definitely a bunch have
  non-useful names and no comments. Several were testing impossible (or,
  definitionally broken) situations like program certificates with no
  corresponding course certificates.
* added django stubs for type checking
* added some type checking

FIXES: APER-3238
* modified a test not only to work with the new code but also to do a
  better job of validating what was looking for
* added mypy
* told coverage not to look for type checking conditionals

FIXES: APER-3238
@deborahgu deborahgu marked this pull request as ready for review June 28, 2024 16:53
@deborahgu deborahgu requested a review from a team as a code owner June 28, 2024 16:53
all tests now use certificate available date (which has another pending
ticket to be removed as a toggle)

upgraded the Python version used to build the documentation because
accessible-pygments  requires a version greater than 3.8.

FIXES: APER-3238
* fixing linting errors
* updating one more github CI python version

FIXES: APER-3238
Comment on lines -259 to -266
def test_get_credentials_program_only(self):
for course_cert in self.course_certs:
course_cert.delete()
for course_run in self.course_runs:
course_run.delete()
course_results, program_results = get_credentials(self.user.username)
assert course_results == []
assert program_results[0] == self.program_user_credential
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was curious as to why we deleted this test case, but after looking at it for a bit... I suppose between the test_get_credentials_course_only and test_get_credentials_both_course_and_program test cases that we're reasonably covered.

(This started as a question that evolved into the comment you see above. I left it here to see if you were working under the same assumption I was)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is mostly what I was looking at, and also, the circumstance where there is a program certificate and yet there are no corresponding courses… What's the correct behavior even supposed to be in that circumstance? I assume both that that is bad data, but also possibly bad data that can happen? But I'd also assume that this test result in the deleted test is wrong -- if you do have a program credential and no corresponding course credentials, I don't believe a program credential should display. Am I wrong?

Copy link
Contributor

@justinhynes justinhynes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for making this dream a bit closer to being a reality.

@deborahgu deborahgu merged commit d9808ca into master Jul 1, 2024
11 checks passed
@deborahgu deborahgu deleted the dkaplan1/APER-3238_remove-visible-date-from-credentials branch July 1, 2024 21:15
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