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

Provide additional logging capabilities #161

Closed
wants to merge 28 commits into from

Conversation

EdStrickland
Copy link
Member

push assertion result to karma

push assertion result to karma
@CLAassistant
Copy link

CLAassistant commented Mar 24, 2020

CLA assistant check
All committers have signed the CLA.

@EdStrickland
Copy link
Member Author

test

@matz3
Copy link
Member

matz3 commented Mar 25, 2020

There are currently problems with the Travis CI infrastructure

lib/client/browser.js Outdated Show resolved Hide resolved
Copy link
Member

@matz3 matz3 left a comment

Choose a reason for hiding this comment

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

Please also add a new integration test scenario.
You can copy one of the folders in test/integration/ and adopt it to use the new configuration property.
Also the result should be asserted in some way (e.g. via the log in the assertions function of the karma.conf.jsfile).

lib/client/browser.js Outdated Show resolved Hide resolved
lib/client/browser.js Outdated Show resolved Hide resolved
@EdStrickland
Copy link
Member Author

Hi @matz3 ,
I am a bit confused when writing the integration tests. It seems some of the tests are dummy tests. Is there any cases that I can refer to when writing cases for my feature?

@matz3
Copy link
Member

matz3 commented Apr 15, 2020

@EdStrickland the idea of the integration tests is to verify the plugin e2e by e.g. checking whether karma was successful and e.g. expecting some console output.

In this case I think you could have some dummy tests with dummy assertions and then check for the log output to contain the expected number of successful tests reported by karma.
Another good test would be to even use a reporter (e.g. cucumber) and then check that the generated report files contain the assertions.

@matz3 matz3 linked an issue Apr 15, 2020 that may be closed by this pull request
@EdStrickland
Copy link
Member Author

Hi @matz3 , I just tried to run the integration tests today but most of them seems to be failing. any ideas?

Async callback was not invoked within the 10000ms timeout specified by jest.setTimeout.Timeout - Async callback was not invoked within the 10000ms timeout specified by jest.setTimeout.Error

@matz3
Copy link
Member

matz3 commented Apr 24, 2020

For me it's working. Maybe it just takes some more time on your machine. Have you tried increasing the timeout to see whether it finishes at some time?

@EdStrickland
Copy link
Member Author

Hi @matz3 ,
The tests have been updated. Would you take a look?

@EdStrickland
Copy link
Member Author

Can someone take a look at the new changes please?

Copy link
Member

@matz3 matz3 left a comment

Choose a reason for hiding this comment

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

Sorry that it took me so long to do a full review

test/integration/application-log-assersion/karma.conf.js Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
lib/client/browser.js Show resolved Hide resolved
lib/client/browser.js Outdated Show resolved Hide resolved
test/integration/application-log-assersion/karma.conf.js Outdated Show resolved Hide resolved
EdStrickland and others added 6 commits May 28, 2020 14:50
…TestRunner.html

Co-authored-by: Matthias Oßwald <1410947+matz3@users.noreply.github.com>
….html

Co-authored-by: Matthias Oßwald <1410947+matz3@users.noreply.github.com>
Co-authored-by: Matthias Oßwald <1410947+matz3@users.noreply.github.com>
1) add new config logHTMLFilePath
2) add new case for the config
@EdStrickland
Copy link
Member Author

Hi @matz3 , how's it going? Anything update on the review?

@matz3
Copy link
Member

matz3 commented Jun 29, 2020

I'm sorry. No progress, yet.

@ecker ecker changed the title Update browser.js Provide additional logging capabilities Jun 29, 2020
@EdStrickland
Copy link
Member Author

Hi @ecker @matz3 , how's it going, is the code ready to go?

@EdStrickland
Copy link
Member Author

Hi @matz3 @ecker , how's everything?

@ecker
Copy link

ecker commented Aug 19, 2020

Hi @EdStrickland, hope you're doing ok. After absences we're gonna catch-up with your PR and keep you updated.

@EdStrickland
Copy link
Member Author

Thanks @ecker, were you on vacation? Hope you had a great time.

@matz3
Copy link
Member

matz3 commented Aug 20, 2020

@EdStrickland I had a deep look into cucumber as I was still unsure about the current state of the report and I would really like to fully understand this requirement / scenario.
For that I've added some rather basic gherkin test to our sample app to check for the reporting:
SAP/openui5-sample-app#89

Unfortunately the report didn't look as I would expect it to be (using both new options)

cucumber report with new options

Here is a report of the same Filter.feature file, based on cucumber-js (Node.js), which looks like the expected state

expected cucumber report based on cucumber-js

Feature: Filtering
  Lorem ipsum

  Background:
    Given I have started the app
    And   I can see 2 items in the list

  Scenario: Filtering for 'Active' items
    When I filter for active items
    Then I can see 1 item in the list

  Scenario: Filtering for 'Completed' items
    When I filter for completed items
    Then I can see 1 item in the list

Could you please have a look into this? Is that what you would also expect?

@EdStrickland
Copy link
Member Author

Hi @matz3 , sorry I was on a vacation. It's been a while so I will need some time to rerun my manual tests to verify.

@EdStrickland
Copy link
Member Author

Hi @matz3 , sorry for the delay. It's months ago since I last replied. Our team was reviewing the possibility of using WDIO instead of OPA. Luckily, OPA stays.

Anyway, I cloned your code and run myself, found a bug and fixed it. and now the logs is displayed as expected in your demo.
By the way, the logHTML option should be turned off to generate cucumber report.

Also, I noticed that we are using SauceLabs to run the tests but it seems to keep failing, would you kindly take a look?

@matz3
Copy link
Member

matz3 commented Oct 22, 2020

Where did you fix the bug? I can't find any related change in the 3 new commits. It's just a merge and fixing eslint issues. Or did I miss something?

Do you mean the logHTMLFilePath option? I've turned it off and the result looks better but there's still the 3rd "Feature" that looks odd (at least the [object Object] doesn't seem to be expected).

image

@matz3
Copy link
Member

matz3 commented Oct 22, 2020

You can ignore the SauceLabs Integration Tests check. It doesn't run for PRs from forks.

@EdStrickland
Copy link
Member Author

You can ignore the SauceLabs Integration Tests check. It doesn't run for PRs from forks.

Fine. Please verify the test results from your side, see if we can merge this PR.

@matz3
Copy link
Member

matz3 commented Oct 23, 2020

Please see my previous comment. I don't think this is ready to be merged.
Also, when you say the logHTMLFilePath option should be disabled, I wonder what the use case for it would be.

@EdStrickland
Copy link
Member Author

Sorry I overlooked the previous comment. I checked my code, and it seems that my latest commit was failed to be pushed. Just pushed the change, please verify.

@EdStrickland
Copy link
Member Author

cucumber

This is what I get with the newest code.

@EdStrickland
Copy link
Member Author

Also, when you say the logHTMLFilePath option should be disabled,

This config was created because karma-ui5 logs the file path in test result. However, the karma-cucumber-reporter would mistake the file path as the feature description and the output cucumber.json would be messy.

@matz3
Copy link
Member

matz3 commented Oct 10, 2023

Hey @EdStrickland!

I'm deeply sorry that there hasn't been any further activities from our side. At some point the work on this contribution has ended, but no communication has been made, as the hope was that it will be continued at some point.

As this unfortunately never happened and the Karma project itself has been deprecated, we will not work on new features, but continue to maintain our plugin until there's an official successor for UI5 testing announced.

@matz3 matz3 closed this Oct 10, 2023
@matz3 matz3 removed their assignment Oct 10, 2023
@matz3 matz3 added the wontfix label Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

detailed test step information in the test result
4 participants