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

ExplanatoryAssertionGroupType Naming #91

Open
jGleitz opened this issue Oct 28, 2020 · 15 comments
Open

ExplanatoryAssertionGroupType Naming #91

jGleitz opened this issue Oct 28, 2020 · 15 comments
Assignees
Milestone

Comments

@jGleitz
Copy link
Collaborator

jGleitz commented Oct 28, 2020

We currently have ExplanatoryAssertionGroupType, as supertype for all group types that add some kind of additional information to an assertion. As of atrium#666, We have these subtypes:

  • ExplanatoryAssertionGroupType
    • WarningAssertionGroupType
    • InformationAssertionGroupType

I want to suggest to redo the naming of these group types based on the use cases we found while discussing #9 and #15.

These use cases are:

  1. tell the user about something that went wrong while checking the assertions, e.g.
    • additional entries in an iterable’s contains check
    • a symbolic link loop while checking a path assertion
    • situations where we cannot extract a feature
  2. give the user context information about a failed assertion. Subject of attach failure hint to existing assertion #9, e.g.
    • giving more information about a file system entry
    • telling the user that although two numbers are not strictly equal, they are numerically equal
  3. tell the user the reason for why the programmer expects an assertion to hold
    • meant for cases where it is not obvious while the assertions where made the way they were

Currently, we map the use cases as follows:

1 -> WarningAssertionGroupType
2 -> ExplanatoryAssertionGroupType
3 -> InformationAssertionGroupType

I see three issues with the current structure:

i. From my point of view, the names of the types are not a good indication for the use case of the types.
ii. I think that it is odd that ExplanatoryAssertionGroupType is a supertype of the other two, although its use case does not really subsum the other two use cases.
iii. I think “warning” is a not a good word choice for WarningAssertionGroupType, as it is always used in a context where the “warning” is not recoverable, i.e. the assertion will not succeed without fixing the warning. To my mind, a “warning” is something bad, but recoverable.

I think we should address these issues. To start the discussion, I suggest the following hierarchy:

  • InformationAssertionGroupType (instead of ExplanatoryAssertionGroupType)
    • RelatedErrorAssertionGroupType (instead of WarningAssertionGroupType)
    • ReasonAssertionGroupType (instead of InformationAssertionGroupType)
    • DebugDetailAssertionGroupType (new type for use case 2)

Most cases where we currently use ExplanatoryAssertionGroupType would use DebugDetailAssertionGroupType. The (new) InformationAssertionGroupType would only be used for cases that are not covered by the uses cases 1, 2 or 3. Ideally, InformationAssertionGroupType would only serve as a fallback type that is never used.

If we decide to rename the group types, we need to make sure to rename all related helpers as well, like ExplanatoryGroup, withExplanatoryAssertion, withExplanation, etc. pp.

@jGleitz
Copy link
Collaborator Author

jGleitz commented Oct 28, 2020

Given that we currently plan to use the name withHelpOnFailure for use case 2, it might make sense to either call the group type HelpAssertionGroupType (instead of DebugDetailAssertionGroupType) or the function withDebugDetailsOnFailure.

@robstoll
Copy link
Owner

Just some food for thought: #1 (comment)

@robstoll
Copy link
Owner

I agree that we should change the names. They represent the currently used symbols and this is actually configurable.

However, renaming ExplanatoryAssertionGroupType to InformationAssertionGroupType only makes sense if we rename ExplanatoryAssertionGroup to InformationAssertionGroup as well. And here I am not sure if this a good name.
I have chosen ExplanatoryAssertionGroup because it is used to explain things, like:

  • explain a defined assertionCreator-lambda in reporting instead of actually evaluating it (used e.g. in expect(listOf(1,2)).all { ... }
  • explain what mismatches where found
  • explain that using BigDecimal.isEqualIncludingScale might not be a good fit
  • explain that path.exists failed because of missing permissions
    etc.

InformationAssertionGroup does not tell me what it is for, its too generic IMO.

@jGleitz
Copy link
Collaborator Author

jGleitz commented Oct 28, 2020

Just some food for thought: #1 (comment)

You have once again proven how bad my memory is 😄

Unsurprisingly, I still like ReportableGroup. Calling the supertype ReportableGroupType would also make sense if its main job is being a fallback supertype that is seldom used directly.

I think this would also address your second comment, wouldn’t it? Removing the word Assertion from all of those groups and group types makes sense, because they are not actually assertions, they’re just something that can be reported.

Most of your examples wouldn’t use ReportableGroupType anyway, they would use the more specific HelpReportableGroupType or DebugDetailReportableGroupType.

It makes sense to my mind.

@jGleitz
Copy link
Collaborator Author

jGleitz commented Oct 28, 2020

To clarify: I think dropping the word “Assertion” from everything that is not actually an assertion already makes sense, even if they still inherit from Assertion. It conveys the intent. We can still realize #1 (i.e. introduce a Renderable interface above Assertion) at a later point.

@robstoll
Copy link
Owner

To clarify: I think dropping the word “Assertion” from everything that is not actually an assertion already makes sense, even if they still inherit from Assertion. It conveys the intent.

I agree

they would use the more specific HelpReportableGroupType or DebugDetailReportableGroupType.

I prefer HelpReportableGroupType over DebugDetailReportableGroupType

Unsurprisingly, I still like ReportableGroup

I am not 100% sure but I think it might be we need ReportableGroup as a super type but I also think that we still need ExplanatoryGroup. I'll give this some thoughts later on

@jGleitz
Copy link
Collaborator Author

jGleitz commented Oct 28, 2020

I am not 100% sure but I think it might be we need ReportableGroup as a super type but I also think that we still need ExplanatoryGroup. I'll give this some thoughts later on.

We definitely need a supertype for the group types, because we use when on them. I currently see no use case that is not covered and hence don’t see what ExplanatoryGroup would be need for. I am looking forward to your thoughts!

@robstoll
Copy link
Owner

robstoll commented Dec 23, 2020

I guess we need to re-iterate this as it seems I don't have the same intuition about the change as you do. I am interested in how you would like to see things. Following a few more examples. In the end, I would like to see the following reports (I introduced a new bullet point 💡)

  • ℹ️ => for additional information provided by the user; currently only via because in the API

  • 💡 failure hints about the usage of the chosen function or rules in Atrium:

    expected subject:             10          (java.math.BigDecimal <1234789>)
    ◆ to equal (including scale): 10.0        (java.math.BigDecimal <1234789>)
      💡 notice, if you used isNumericallyEqualTo then the assertion would have hold.
    

    //

    expected subject: [1]        (java.util.Collections.SingletonList <1234789>)
    ◆ ▶ get(0): 1                (kotlin.Int <1234789>)
        ◾ at least one assertion defined: false
           💡 You forgot to define assertions in the assertionCreator-lambda
              Sometimes you can use an alternative to `{ }` 
              For instance, instead of `toThrow<..> { }` you should use `toThrow<..>()
    

    //

    expected subject: MyClass
    ◆ to equal:       MyClass
        💡 notice, MyClass has not overridden equals
           better use toBeSameInstanceAs or implement equals/hashCode
    
  • 🔎 additional information to help debugging the failure

    expected subject: /usr/bin/noprogram        (sun.nio.fs.UnixPath <1234789>)
    ◆ to: exist
      🔎 the closest existing parent directory is /usr/bin
    

    //

    expected that subject: /tmp/atrium-path/directory/subfolder/file        (sun.nio.fs.UnixPath <1234789>)
    ◆ is: a file
      🔎 followed the symbolic link /tmp/atrium-path/directory to /tmp/atrium-path/file
        ❗❗ failure at parent path: /tmp/atrium-path/file
          ⚬ was a file instead of a directory
    
  • ❗❗to indicate an error or further specify the error

    expected that subject: {a=1, b=2}        (java.util.LinkedHashMap <1234789>)
    ◆ contains, in any order:
      ⚬ ▶ entry "c": ❗❗ key does not exist
          » to equal: 2                     (kotlin.Int <1234789>)
      ⚬ ▶ entry "b":  2                     (kotlin.Int <1234789>)
          ◾ equals:  1                     (kotlin.Int <1234789>)
    

    //

    expected that subject: {a=1, b=2}        (java.util.LinkedHashMap <1234789>)
    ◆ ▶ size:      2                         (kotlin.Int <1234789>)
        ◾ equals: 1                         (kotlin.Int <1234789>)
    ◆ contains only, in any order:
      ✔ ▶ entry "b":  2                      (kotlin.Int <1234789>)
          ◾to equal: 2                       (kotlin.Int <1234789>)
        ❗❗ additional entries detected:
           ⚬ entry "a": 1                    (kotlin.Int <1234789>)
    

    // (also note how I would use 🔎 in this scenario)

    expected subject: /root/.ssh/config        (sun.nio.fs.UnixPath <1234789>)
    ◆ to be: writable
      ❗❗ failure at parent path: /root        (sun.nio.fs.UnixPath <1234789>)
         ⚬ access was denied
        🔎 the owner is root, the group is root
        🔎 the permissions are u=rwx g= o=
    
  • » to indicate an expectation the user defined but could not be evaluated as a previous subject transformation failed

    expected that subject: {a=1, b=2}        (java.util.LinkedHashMap <1234789>)
    ◆ contains, in any order:
      ⚬ ▶ entry "c": ❗❗ key does not exist
          » to equal: 2                     (kotlin.Int <1234789>)
      ⚬ ▶ entry "b":  2                     (kotlin.Int <1234789>)
          ◾ equals:  1                     (kotlin.Int <1234789>)
    

@robstoll
Copy link
Owner

Once we tackle the alignment in reporting, we will need to look for bullet points with a standard width as it really looks ugly if the numbers or parentheses are slightly shifted.

@jGleitz
Copy link
Collaborator Author

jGleitz commented Dec 23, 2020

Thanks for writing down all those examples! I like your idea of differentiating between messages from the library and messages from the programmer by using ℹ️ or 💡, respectively.

I agree with all your examples. My only disagreement is with the properties of exceptions, but as I stated in #738, I can live with it, as long as it is not ℹ️. With the new meaning of ℹ️, this because even more important.

I think it is important that we chose good names for the groups representing these bullet points. We should make it as easy as possible for developers to understand what the groups are good for.

@jGleitz
Copy link
Collaborator Author

jGleitz commented Dec 23, 2020

we will need to look for bullet points with a standard width

is that something we can influence? I’d expect that the character width is determined by the user’s font 😕 .

We could use tabs, which would give as reliable alignment. We would lose control over the width of spacings, though.

@robstoll
Copy link
Owner

is that something we can influence? I’d expect that the character width is determined by the user’s font confused .

At least a bit. I am only talking about the real bullet points: ◆⚬◾ and not about the emojis; there we are definitely lost.
The ones I have chosen now are sometimes a bit wider and I used a smaller space to compensate it. It worked quite fine I guess (because no one ever complained) but it would not once we align it.

@vlsi
Copy link
Collaborator

vlsi commented Feb 20, 2023

How do all these emojis look on Windows console that does not support UTF-8?

@robstoll
Copy link
Owner

Good point, so far we did not target windows consoles and as there was never a complaint, I guess most use linux runners in CI?
I am aware of that even the current bullet points we use don't look nice in CMD or similar. Personally, I just ignored it in the CI windows built of Atrium.

I guess we never got a complaint because most users use intellij and it looks all good on their console.

Nevertheless, it's something we should tackle if we have more users who use Windows (but IMO it's not a priority). We plan to introduce colours via ANSI and there we will have a fallback for terminals not supporting ANSI (i.e. also all the Windows consoles).
I already started with a draft once and as far as I remember, I used asci symbols instead of emoij if there was no support for ANSI.

Do you know if this is coupled, in other words, is it enough if we base the emoji fallback on the support for ANSI or should we somehow figure out in another way the (proper) support for UTF-8?

I would pre-seen the following ascii icons:

  • ℹ️ => (i)
  • 💡 => (u)
  • 🔎 => (d)
  • ❗❗=> (!)

@vlsi
Copy link
Collaborator

vlsi commented Feb 20, 2023

CI windows

That is my case :-/

is it enough if we base the emoji fallback on the support for ANSI or should we somehow figure out in another way the (proper) support for UTF-8?

I guess the only workaround, for now, is to use ASCII

@robstoll robstoll modified the milestones: 0.17.0, 2.0.0 Apr 21, 2023
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

No branches or pull requests

3 participants