represent various objects when logging better #98

bandophahita · 2023-08-23T20:55:02Z

This is a fix for the way different objects are represented when logging.

strings will use repr.
matcher objects that have a describe_to function will describe themselves.
all other objects will surrounded the __str__ output with angle brackets <1>, <True>, <MyCustomObj>

perrygoy

I have a lot of questions below, i'm not sure i fully understand the purpose of these changes.

screenpy/resolutions/contains_the_item.py

screenpy/resolutions/custom_matchers/sequence_containing_pattern.py

screenpy/speech_tools.py

tests/test_resolutions.py

perrygoy

Sorry, i had a few more things that i think we should address on this one before merging.

I now fully understand the reasoning and the benefit of adding this speech tool, and thank you for doing all this work for it! I think it could be the first of several narration-massaging tools that we could offer our users in the speech tools module.

tests/test_actions.py

perrygoy · 2023-09-08T17:17:36Z

tests/test_actions.py

+        assert [r.msg for r in caplog.records] == [
+            "Tester sees if all of 4 tests pass:",
+            "    Tester sees if fakeQuestion is equal to <True>.",
+            "        ... hoping it's equal to <True>.",
+            "            => <True>",
+            "    Tester sees if fakeQuestion is equal to <False>.",
+            "        ... hoping it's equal to <False>.",
+            "            => <False>",
+            # don't be fooled! the next few lines do not have commas on purpose
+            "        ***ERROR***\n"
+            "\n"
+            "AssertionError: \n"
+            "Expected: <False>\n"
+            "     but: was <True>\n",


I don't think this test belongs in this test class. I think we should put it in TestSee.

I do think it's worth it to have a test that each of the Actions which now use represent_prop are representing the props how we want them to be represented, but SeeAllOf doesn't directly use it.

I think this test isn't only confirming that represent_prop does the right thing. It's a confluence of everything that goes into making SeeAllOf work as expected.

beat messages have to format and indent properly

settings have to work properly

See has to function properly

SeeAllOf has to loop through each of the passed in tests.

I think of it like this -- we want to prove SeeAllOf calls See, we can do that with another test showing how many times it gets called. But say someone tries to sneak in the following change:
the_actor.should(Silently(See.the(question, resolution)))
The test with the number of calls wouldn't catch that whereas this test would.

I dunno, i think it's still in the wrong place and is (well, would be—we don't have the test that i'm suggesting in TestSee) testing things that are already tested elsewhere.

I personally tend towards not having tests that are a group of tests we already have unless that integration is testing something new. I don't see anything new being tested with this test, aside from the test that is missing from TestSee of course.

In principle I agree that unit tests should be as atomic as possible. I've always struggled with testing functionality that basically coalesces multiple functionalities. Each of the smaller functions have been tested but how to you prove they all came together in the right manner? I found that at some point you'll get some overlap.

"We aren't proving the sugar is sweet, we are proving we used it in the recipe."

I added two tests to See to prove the logging works there as expected.

For me, i want to know why a test at this level failed just by reading the title, if possible.

I think you might like functional testing, which test things as they would be used by a user, rather than all this fiddling with unit tests. So instead of testing each part down here, we'd have a test that was like:

def test_click_does_a_click(self, Tester): target = Target.the("test").located_by("//test") Tester.attempts_to(Click.on_the(Target)) # assert the dummy element was asked for by "//test" # assert the dummy element had `.click()` called

It's a pretty interesting concept that turns the tests into more documentation on how the developers expect the tool to be used. If you remember a few years ago, the tests in here used to be kind of like that, until i realized i could just do Click().perform(Tester) instead and skipped a layer.

I think the tests as they are now are easier to organize, but i did kind of like how those older tests looked.

I can't find the article now that introduced me to the concept because i specifically remember it going on at length about how neat it is that someone wanting to use your library can just look at your tests directory to see how to use it. :(

I do remember i had found it right after i finished changing all the unit tests to how they are now, though, so i didn't want to change them all back. Haha.

These logging tests are probably closer to functional rather than unit tests. I think the trick to any test suite (especially a public library) is no one way will suffice. We want as comprehensive as coverage as we can get.

For instance, if we were to completely rewrite SeeAllOf to no longer use See but we still wanted the logging to output the same, we need a test to show that it works as expected. This treats the action like a black box. There may be overlap at a unit test level, but only because we know what's inside that action.

I want to know why a test at this level failed just by reading the title,

Perhaps I should rename these tests. Something like test_log_first_failure ?

tests/test_actions.py

tests/test_resolutions.py

perrygoy · 2023-09-08T17:23:28Z

tests/test_speech_tools.py

+    def test_int(self):
+        val = 1234
+
+        assert represent_prop(val) == "<1234>"


If we wanted to, we could parametrize this as test_everything_else and verify that a bunch of different classes get wrapped in the <>s.

You're not wrong...

Separating them gives you a clear indication of where a bug lies. If somehow we only break how ints are displayed, when we run the tests TestRepresentProp:test_int will inform us. While in this case that's not a significant difference in how we would go about fixing it, it is often helpful to have that separation to quickly diagnose a bug.

Code wise it's a couple extra lines. /shrug

Parametrized tests still show what the values are, so we'd have that same level of granular feedback. We can also give each parameter'd test a specific name if we'd like to.

Fair enough. I updated them.
I have a tendency to only use parameterized tests when there are 3 or more variations.

perrygoy

Some final thoughts and suggestions, but they're no longer blocking.

I'm excited to get this one in!

perrygoy · 2023-09-23T01:20:14Z

screenpy/actions/make_note.py

@@ -54,9 +55,9 @@ def as_(self: SelfMakeNote, key: str) -> SelfMakeNote:

    def describe(self: SelfMakeNote) -> str:
        """Describe the Action in present tense."""
-        return f"Make a note under {self.key}."
+        return f"Make a note under {represent_prop(self.key)}."


This could be key_to_log instead, just in case we change our mind on represent_prop.

(And it looks like that's the approach you have in the other describe methods, to use *_to_log.)

I've always worried that setting the variable in the constructor would somehow lead to a problem if anyone were to alter the attribute being logged after instantiation. It might be a non-issue but it's one of the reasons I try to avoid it as much as possible.

We want the value coming out of describe to be accurate at the moment it is called.

I forgot that part of the reason we have *_to_log is for the beat message. I kinda want to go through each of the classes and make those attributes properties.

I think they will not need to be around any longer, ideally, once we figure out this whole update to the logging bit. Or maybe the description/beat itself becomes a property, i think that would be slick.

Either way, let's do the describe reimagining before we do a change like that, because it might not be necessary in that brave new world.

I think that would be slick.

It really would. the only reason I didn't earlier is because the pattern wasn't already being used. I'll open an issue to add those at some point.

perrygoy · 2023-09-23T01:24:37Z

tests/test_actions.py

+        assert [r.msg for r in caplog.records] == [
+            "Tester sees if all of 4 tests pass:",
+            "    Tester sees if fakeQuestion is equal to <True>.",
+            "        ... hoping it's equal to <True>.",
+            "            => <True>",
+            "    Tester sees if fakeQuestion is equal to <False>.",
+            "        ... hoping it's equal to <False>.",
+            "            => <False>",
+            # don't be fooled! the next few lines do not have commas on purpose
+            "        ***ERROR***\n"
+            "\n"
+            "AssertionError: \n"
+            "Expected: <False>\n"
+            "     but: was <True>\n",


I dunno, i think it's still in the wrong place and is (well, would be—we don't have the test that i'm suggesting in TestSee) testing things that are already tested elsewhere.

I personally tend towards not having tests that are a group of tests we already have unless that integration is testing something new. I don't see anything new being tested with this test, aside from the test that is missing from TestSee of course.

tests/test_resolutions.py

perrygoy

Re-approving! Thanks for the extra TLC!

…mat.

bandophahita · 2023-09-28T14:10:06Z

@perrygoy I updated those tests and rebased with trunk. I'm sorry to make you review it again.

perrygoy

Re-re-re-re-approving! I've got more where that came from.

* initial deps * initial project sync (via cruft) * ruff ANN * updated test based on changes in ScreenPyHQ/screenpy#98 * ruff ARG * ruff D * ruff EM * ruff FA * ruff I * ruff TCH * ruff PGH * ruff PIE * ruff PT * ruff RUF * ruff SIM * ruff UP * black formatting * fixing mypy namespace error * adding copyright tests * adding namespace test * fixing name of mocked item since it's being patched at the class level.

perrygoy reviewed Aug 24, 2023

View reviewed changes

bandophahita force-pushed the loggin_strings branch from 430847f to 0b91ae5 Compare August 24, 2023 16:12

bandophahita changed the title ~~show the repr of a string object when logging~~ represent various objects when logging better Aug 24, 2023

perrygoy reviewed Sep 8, 2023

View reviewed changes

MarcelWilson force-pushed the loggin_strings branch from 0f6bffa to 6bcd0c2 Compare September 22, 2023 01:08

perrygoy previously approved these changes Sep 23, 2023

View reviewed changes

bandophahita dismissed perrygoy’s stale review via ae43bd3 September 25, 2023 17:33

perrygoy previously approved these changes Sep 26, 2023

View reviewed changes

bandophahita dismissed perrygoy’s stale review via 4b59c5e September 28, 2023 13:33

bandophahita added 18 commits September 28, 2023 09:04

show the repr of a string object when logging

8992612

forgot you can't use the 'and' syntax before python 3.10

1417aa3

This line should not have changed. I must have accidentally hit refor…

9e9c247

…mat.

making adjustments based on feedback

2f4b20a

fixing ContainsTheEntry to use tostring

979f14c

using explicit strings in tests is better.

530f25d

renaming function to represent_prop

9141a57

Missed a case

f8c2b47

rename test to match the function tested

ccaa005

represent all other objects with angle brackets (like hamcrest does)

21f436e

adding case for MakeNote

d6d9052

additional coverage of logging

db06f32

test formatting

7bdce54

adding logging tests for See

9ddf365

Updating tests based on PR comments

8ff51bb

Missed some cases

83f5281

better descriptive names of tests

d6e611a

parameterizing tests per PR comments

860ab64

bandophahita force-pushed the loggin_strings branch from 34b9a96 to 860ab64 Compare September 28, 2023 14:04

fixing tests after rebased latest trunk changes

3663847

perrygoy approved these changes Sep 28, 2023

View reviewed changes

bandophahita merged commit 72481d8 into ScreenPyHQ:trunk Oct 4, 2023
9 checks passed

bandophahita deleted the loggin_strings branch October 9, 2023 23:54

bandophahita added a commit to bandophahita/screenpy_adapter_allure that referenced this pull request Feb 19, 2024

updated test based on changes in ScreenPyHQ/screenpy#98

69ac550

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

represent various objects when logging better #98

represent various objects when logging better #98

bandophahita commented Aug 23, 2023 •

edited

Loading

perrygoy left a comment

perrygoy left a comment

perrygoy Sep 8, 2023

bandophahita Sep 22, 2023

bandophahita Sep 22, 2023

perrygoy Sep 23, 2023

bandophahita Sep 25, 2023

bandophahita Sep 25, 2023

perrygoy Sep 26, 2023 •

edited

Loading

perrygoy Sep 26, 2023 •

edited

Loading

bandophahita Sep 28, 2023 •

edited

Loading

bandophahita Sep 28, 2023 •

edited

Loading

perrygoy Sep 8, 2023

bandophahita Sep 22, 2023

perrygoy Sep 23, 2023

bandophahita Sep 28, 2023

perrygoy left a comment

perrygoy Sep 23, 2023

perrygoy Sep 23, 2023

bandophahita Sep 25, 2023

bandophahita Sep 25, 2023

perrygoy Sep 26, 2023

bandophahita Sep 28, 2023

perrygoy Sep 23, 2023

perrygoy left a comment

bandophahita commented Sep 28, 2023

perrygoy left a comment

represent various objects when logging better #98

represent various objects when logging better #98

Conversation

bandophahita commented Aug 23, 2023 • edited Loading

perrygoy left a comment

Choose a reason for hiding this comment

perrygoy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perrygoy Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

perrygoy Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

bandophahita Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

bandophahita Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perrygoy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perrygoy left a comment

Choose a reason for hiding this comment

bandophahita commented Sep 28, 2023

perrygoy left a comment

Choose a reason for hiding this comment

bandophahita commented Aug 23, 2023 •

edited

Loading

perrygoy Sep 26, 2023 •

edited

Loading

perrygoy Sep 26, 2023 •

edited

Loading

bandophahita Sep 28, 2023 •

edited

Loading

bandophahita Sep 28, 2023 •

edited

Loading