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

[cucumber-jvm] Add attachments to allure reports using cucumber api #339

Merged
merged 7 commits into from
Apr 15, 2019
Merged

[cucumber-jvm] Add attachments to allure reports using cucumber api #339

merged 7 commits into from
Apr 15, 2019

Conversation

PavloNikolenko
Copy link
Contributor

@PavloNikolenko PavloNikolenko commented Mar 29, 2019

Context

This PR allows users of cucumber + allure to add attachments using cucumber api:
scenario.write("Text comment")
scenario.embed(byte[], "image/png")

Cucumber documentation: https://docs.cucumber.io/cucumber/api/#after

Checklist

@CLAassistant
Copy link

CLAassistant commented Mar 29, 2019

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@baev baev left a comment

Choose a reason for hiding this comment

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

👍

Could you please add a tests?

@PavloNikolenko
Copy link
Contributor Author

Sure. I'll work on it.

@PavloNikolenko
Copy link
Contributor Author

Ignore that last commit. Just fixing github user issue.

@PavloNikolenko
Copy link
Contributor Author

@baev Please review and I will add another unit test for scenario.embed

Copy link
Member

@baev baev left a comment

Choose a reason for hiding this comment

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

LGTM in general. Please add tests for both methods to all adaptor versions

@PavloNikolenko
Copy link
Contributor Author

PavloNikolenko commented Apr 4, 2019

@baev Please review when you get a chance

@baev
Copy link
Member

baev commented Apr 11, 2019

@PavloNikolenko could you please resolve conflicts

@PavloNikolenko
Copy link
Contributor Author

Resolved

@baev
Copy link
Member

baev commented Apr 11, 2019

[Sizes | LineLength] io.qameta.allure.cucumberjvm.(AllureCucumberJvm.java:263)
Line is longer than 120 characters (found 133).
http://checkstyle.sourceforge.net/config_sizes.html#LineLength

@PavloNikolenko
Copy link
Contributor Author

Do you want me to update that?

@baev
Copy link
Member

baev commented Apr 11, 2019

@PavloNikolenko yep, the build should pass before I can merge it

@PavloNikolenko
Copy link
Contributor Author

Can you rebuild that job manually? Looks like something went wrong.

@PavloNikolenko
Copy link
Contributor Author

Why is it failing?

@PavloNikolenko
Copy link
Contributor Author

I found the problem:
this before step

@Before
public void setup(Scenario scenario)
{
    this.scenario = scenario;
}

is executed for all scenarios and messes up some other unit tests

@PavloNikolenko
Copy link
Contributor Author

Hey @baev. Its all green now. Please take a look when you get a chance.

@baev baev merged commit f60fadf into allure-framework:master Apr 15, 2019
@baev
Copy link
Member

baev commented Apr 15, 2019

@PavloNikolenko thanks! 👍

@PavloNikolenko
Copy link
Contributor Author

Thank you =)

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.

3 participants