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

asterisks displaying instead of single quotes in Command Log assertion #1360

Open
jennifer-shehane opened this Issue Feb 21, 2018 · 6 comments

Comments

4 participants
@jennifer-shehane
Member

jennifer-shehane commented Feb 21, 2018

  • Cypress Version: 2.0.3
  • Browser Version: Crome

Is this a Feature or Bug?

Bug

Current behavior:

Command Log for assertion chain includes confusing asterisks. I suspect it may be replacing \n with **.

screen shot 2018-02-21 at 2 24 08 pm

Even displays in passing tests
screen shot 2018-02-21 at 2 27 27 pm

Desired behavior:

Error in Command Log should display as:

expected '<div.duration>' to have text '37:46', but the text was ' 37:46 '

Code to reproduce

cy.get(".duration").should("include.text", "37:46")
@chrisbreiding

This comment has been minimized.

Collaborator

chrisbreiding commented Feb 21, 2018

We send the values down wrapped with ** so they'll get processed with markdown and turned into <strong> tags. The problem is that when there's a newline between the **s, markdown doesn't recognize the **s as belonging together.

We need to escape newlines so they actually appear like \n37:46. That will make it more obvious to the user too why the assertion failed.

@srwiseman

This comment has been minimized.

srwiseman commented Feb 24, 2018

Hi, I'm new to contributing, but I don't think the problem is specifically due to newlines. It's the fact that the entire string itself is being processed as markdown. I don't think that's a good idea.

In this specific case, the issue is that markdown treats '** 37:46 **' as ** 37:46 **, not ' 37:46 ' It's because in markdown, you can't put a bold tag in front of white space.

However, this issue goes deeper than this.

Let's say I'm looking to match _37:46_:

cy.get(".duration").should("include.text", "_37:46_")

Now we have italicized our test

image

Let's take it to the extreme, let's say we do this:

cy.get(".duration").should("include.text", "![cypress](https://www.cypress.io/img/logo-dark.4e8064a6.png)")

We get this:

image

Basically, if your tests include anything that can be parsed by markdown, it will. I have not looked at where specifically in the code these strings are being processed by markdown, but I think it should be avoided (or at least the contents of these strings should not be parsed at all).

Here's another example of how this could be really confusing:

Let's look for *37:46*:

cy.get(".duration").should("include.text", "*37:46*")

Now we get this:

image

This makes no sense at all since it's saying that two identical strings don't match. It should show exactly what it does further down:

image

My conclusion would be that these strings shouldn't be parsed by markdown at all. It would also make sense to add quotes so that it would be more obvious why the two strings don't match.

I'd be happy to investigate the code and make these changes, but I'd imagine a design change like this would have to be signed off by someone.

@brian-mann

This comment has been minimized.

Member

brian-mann commented Feb 25, 2018

We originally tried quotes but then it looked like everything you were comparing were strings.

Remember that you can always click on an assertion and it will print the actual values to the console completely unformatted.

Stringifying any kind of value will ultimately format it potentially different than the way its data structure actually is. Comparing function references has the same problem.

I think all we need to do in this situation is escape the values so that they will not be parsed or rendered as markdown - but we can otherwise continue using it. It's a fairly trivial solution.

@chrisbreiding can you knock this one out?

@srwiseman

This comment has been minimized.

srwiseman commented Feb 25, 2018

Interesting. That would work if you escape every markdown special character, but isn't one of the issues here that if the string starts with whitespace, it will also display incorrectly? for instance, ' Test ' will display as '** Test **' since you cannot start a bolded section in markdown with whitespace (as shown in the first comment). You also cannot escape the whitespace in markdown (as far as I can see).

What would be the solution for that? Would you strip the whitespace from the front and back of each string as well as escaping special characters?

Also, I noticed that this was tagged as first-timers-only. Sorry if I'm overstepping, but it would be awesome if I could help out with a solution here :). I really like cypress, and I'd love to get involved with some open source!

Steve

@brian-mann

This comment has been minimized.

Member

brian-mann commented Feb 25, 2018

We could add quotes if the assertion subject or expected value is a string. That would fix the bold issue - since as long as quotes come after the asterisks then you can subsequently have spaces. That would also provide more context on the assertion subject and values (like chrome dev tools does). But other data structures should not be quoted - such as numbers, arrays, objects, etc.

For other characters and new lines we likely still need to escape those so they are not literally evaluated by markdown.

The tests for the assertion messages are here: https://github.com/cypress-io/cypress/blob/develop/packages/driver/test/cypress/integration/commands/assertions_spec.coffee

You're more than welcome to start on a PR. Most of the time our team will "top off" or finish PR's from outside contributors. Having an initial PR will definitely get the ball rolling and we prioritize those over issues that are "ready for work". We do have good contributing docs - but Cypress is a large and complex project - so we don't expect people to get everything right by themselves.

Because we are a team working on this every day - in some cases there are clear bug fixes where the solution does not require discussion - in other cases (like this one) the actual solution may not be clear. In those cases, we may iterate internally as a team to quickly come to a decision. Oftentimes there are areas of the system that some team members are more familiar with than others. In this case - @chrisbreiding was the original person who implemented markdown in order to structure the assertions.

My thought process on this is that bolding the subjects and the values is the most common and correct use case. It's ideal but as you've pointed out - there are edge cases. Instead of replacing the system, I believe it would be better to handle those edge cases and further stick to the 80/20 rule. It doesn't have to be perfect - you can always click on an assertion to see its true valuel

I think the current discussion is about handling:

  • strings (suggesting we quote them)
  • non strings (primitives)
  • object structures (arrays, functions, objects)
  • other characters like new lines, tabs, etc

We have a set of common stringifier utils in the driver that are used throughout the system that handle stringification. We likely just need to utilize those.

@srwiseman

This comment has been minimized.

srwiseman commented Feb 26, 2018

Cool! Thank you for the detailed explanation of your workflow! I look forward to contributing if I see the opportunity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment