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

Scenario Outlines with example params in title break report formatting #3522

Open
eitzenbe opened this issue Sep 3, 2024 · 15 comments
Open

Comments

@eitzenbe
Copy link
Contributor

eitzenbe commented Sep 3, 2024

What happened?

Cloned serenity-cucumber-starter project, upgraded to serenity version 4.1.20 (but also fails with original version)

changed feature file to:

Feature: Search by keyword

@green
Scenario: Searching for 'green'
Given Sergey is researching things on the internet
When he looks up "green"
Then he should see information about "green"

@red
Scenario Outline: Searching for ""
Given Sergey is researching things on the internet
When he looks up ""
Then he should see information about ""
Examples:
|color|
|blue |

Report looks strange:

image

First entry is from a version where the title was without "<color" but only in title

image

Side Note: This happens only on testresults pages, on the feature pages its all fine:

image

image

What did you expect to happen?

The Testresults pages/tables show the title with correctly escaped "<" and ">" tags

Serenity BDD version

4.1.20

JDK version

jdk17

Execution environment

Linux

How to reproduce the bug.

see above, minimal changes to be applied to serenity-cucumber-starter project

How can we make it happen?

Work on this myself and propose a PR (with Serenity BDD team guidance)

@wakaleo
Copy link
Member

wakaleo commented Sep 3, 2024

Well spotted - the thing to do would be to check the reporting logic for the feature files (start with the freemarker templates) and see what text transformation functions are used there. You should be able to just replicate these in the other template.

I don't have the code handy but if you can't find the right template let me know and I will find it when I have access.

@eitzenbe
Copy link
Contributor Author

eitzenbe commented Sep 3, 2024

mvn install -DskipTests running ;)

@wakaleo
Copy link
Member

wakaleo commented Sep 3, 2024

Yep, you can test a snapshot build that way (don't run the full test suite each time, as the full test suite takes a while and is a bit Linux-centric for some of the tests).

@eitzenbe
Copy link
Contributor Author

eitzenbe commented Sep 3, 2024

I am off course on linux ;)

@eitzenbe
Copy link
Contributor Author

eitzenbe commented Sep 3, 2024

@wakaleo PR avail for review, I did a minimal invasive fix, which might not be the most elegant way, but I don't want to open up pandoras box here ;) I tested with scenario titles containing <,>," as these would fuck the report HTML the most and it seems you are checking for a lot of special chars but not these

@eitzenbe
Copy link
Contributor Author

eitzenbe commented Sep 4, 2024

@wakaleo Tried to reach out to you but reachme@.... seems to be not working

This is the mail system at host eitzen.at.

I'm sorry to have to inform you that your message could not
be delivered to one or more recipients. It's attached below.

For further assistance, please send mail to postmaster.

If you do so, please include this problem report. You can
delete your own text from the attached returned message.

               The mail system

[email protected]: host aspmx.l.google.com[173.194.76.27] said: 550-5.1.1
The email account that you tried to reach does not exist. Please try
550-5.1.1 double-checking the recipient's email address for typos or
550-5.1.1 unnecessary spaces. For more information, go to 550 5.1.1
https://support.google.com/mail/?p=NoSuchUser
5b1f17b1804b1-42bb6deacbdsi70759145e9.19 - gsmtp (in reply to RCPT TO
command)

@wakaleo
Copy link
Member

wakaleo commented Sep 4, 2024

This should be good - I will take a look this evening

@eitzenbe
Copy link
Contributor Author

eitzenbe commented Sep 4, 2024

sent message via l...in

@wakaleo
Copy link
Member

wakaleo commented Sep 4, 2024

There are some failing tests in WhenFormattingDataForTheHTMLReports.groovy and WhenFormattingForHTML.java - they need to be updated to reflect the new behaviour, or in some cases they may indicate regressions in other behaviour that need to be integrated in your code. You can run these tests in isolation

e.g.
image

@eitzenbe
Copy link
Contributor Author

eitzenbe commented Sep 4, 2024 via email

@eitzenbe
Copy link
Contributor Author

eitzenbe commented Sep 5, 2024

@wakaleo Hmmm the method htmlCompatible in the formatter class should escape all special characters assuming it is a text that is put in or? If so the test case is wrong, without javadoc its hard to see whether the test is wrong or the new method impl I did. From the name "plainHtmlCompatible" I deducted that < and > characters should be escaped too as these could lead to problems in the title section of scenarios in the report.

The test method name implies you are actually testing the renderMarkdown method in which case I would just remove the chained call to htmlCompatible., What do you say?

br
Thomas

@wakaleo
Copy link
Member

wakaleo commented Sep 5, 2024

The test method names are usually a good indication of the indented behaviour. Sometimes we don't want to escape < and > because we actually do want to render the HTML as is (particularly if we are allowing markdown rendering such as is the case for test titles and descriptions). For example, this one looks like there is an issue rendering the markdown notation:
image

@eitzenbe
Copy link
Contributor Author

eitzenbe commented Sep 5, 2024

reworking the code to not touch the formatter classes at all, PR incoming ;)

@eitzenbe
Copy link
Contributor Author

eitzenbe commented Sep 5, 2024

Ok so fixed it with staying inside ftl to avoid side effects, but WHY are you replacing "<" in example table values with "{" this may cause "havoc" if test data on purpose contains a "<"... Not going down that rabbit whole but might be a good idea to rework that at a later time ;)

image

@wakaleo
Copy link
Member

wakaleo commented Sep 5, 2024

Ok so fixed it with staying inside ftl to avoid side effects, but WHY are you replacing "<" in example table values with "{" this may cause "havoc" if test data on purpose contains a "<"... Not going down that rabbit whole but might be a good idea to rework that at a later time ;)

image

Thanks. This is a good point and there was probably a very good reason at the time which I don't recall now 🤷

Did you raise a PR?

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

2 participants