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

Fixes for tests in 13 #257

Merged
merged 1 commit into from
Jun 13, 2024
Merged

Fixes for tests in 13 #257

merged 1 commit into from
Jun 13, 2024

Conversation

hjwp
Copy link
Owner

@hjwp hjwp commented Jun 12, 2024

The tests here were failing because a couple of changes we made to the listings were not in sync witht he commit history of the branch in the book-example repo. Here's how I fixed things at a high level:

  • first, determine that the cause was due to the commits getting out of sync. I saw the error was that the Fts were failing when the book said they should be passing, and then I see it's at a part of the book where we say "now remove the early return", and I realised we didn't have a commit where we remove the early return.
  • I then realised we didn't have a commit for when we added the early return either! that's where we had the commit listing as ch13l???.
  • so I first added a commit for when we introduce the early return. I did this by:
  1. checkout the chatper 13 branch
  2. reset --hard to the commit just before where we want to add the early return code
  3. add the early return, and make a new commit for it. I put --ch13l030-3-- at the end of the commit message
  4. update the book to show this "commit id" (i also tweaked the comment message while i was at it)
  5. git cherry-pick all the rest of the commits from the old commit history

that procedure, reset --hard <commit N> + commit <new commit N+1> + git cherry-pick <old-commit-N+1...end> is one i use quite often. it's a bit like a rebase, rewriting history.

Then I followed a similar procedure to introduce an extra commit for when we remove the early return as well. I added that to the book chapter text too.

Finally I noticed a problem with applying one of the later commits, it couldn't apply the commit patch successfully. I managed to figure out that was because of the change where we swapped the order of form.save() and form.full_clean(). SO for that I did a git rebase --interactive and marked the commit where we introduce that form.full_clean() for editing, and changed it to match the book.

that gave me a conflict at the end of the rebase, which is expected, and so i fixed that..... i'm aware this explanation is getting a little abstract probably. but i thought it was good to write it down.

happy to go over this sort of procedure in detail with you one of these days! there just never seems to be enough timeeeeeeee. but, you have to invest time to make time! so.

@@ -598,7 +599,7 @@ Let's put an early return in the FT to separate
what we got working from those that still need to be dealt with:

[role="sourcecode"]
.src/functional_tests/test_list_item_validation.py (ch13l???)
.src/functional_tests/test_list_item_validation.py (ch13l030-3)
Copy link
Owner Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is related to #240

@@ -608,8 +609,7 @@ class ItemValidationTest(FunctionalTest):
self.browser.find_element(By.ID, "id_new_item").send_keys(Keys.ENTER)
self.wait_for_row_in_list_table("1: Buy milk")

return
# TODO fix the remaining test scenarios
return # TODO re-enable the rest of this test.
Copy link
Owner Author

Choose a reason for hiding this comment

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

decided to tweak this listing slightly. sorry i'm a micromanager! 😅

selenium.common.exceptions.NoSuchElementException: Message: Unable to locate
element: .invalid-feedback; [...]
[...]
AssertionError: '2: Use peacock feathers to make a fly' not found in ['1: Buy
Copy link
Owner Author

Choose a reason for hiding this comment

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

a nice side-effect of this fix - the book tests failed on this bit, saying it couldn't find this part of the traceback. sure enough, it's because this error was coming from the FT that we've put an early return in, so it doesn't actually appear any more.

this is exactly what the book tests are for, yay!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh that's a nice catch!

-
# Perversely, she now decides to submit a second blank list item
----
====
Copy link
Owner Author

Choose a reason for hiding this comment

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

i thought i'd add the listing explicitly here. this causes the book tests to actually try and apply the code change.

there's also a magic way of applying commits by adding role=dofirst-ch13l0xx to the asciidoc source, but that's cheating. this is nice and exlicit.

also - sometimes it's nice to use source,diff as the listing type, and show a diff rather than a python syntax highlighted codeblock. works well here.

@Xronophobe Xronophobe merged commit 6502ee1 into main Jun 13, 2024
11 of 12 checks passed
@hjwp hjwp deleted the fix-13-tests branch June 14, 2024 20:36
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