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

various updates (#23) #59

Conversation

vveliev-tc
Copy link

  • Set description to cmd args of parallel_tests process

  • Fix matcher to detect when start time needs to be updated

  • Clean up formatting in report.rb

  • Implement a folder creation lock file to address duplicate folders being created

  • Delay each process by its ID number to create a staggered start to reporting in Report Portal (addresses load issues)

  • Formatting and cleanup in parallel_report.rb

  • Use temporary directory for tracking folder creation

  • Add commenting around why we are delaying threads

  • Make sure to use a "full path" for folder creation tracking

  • fixing report description when run in parallel

  • refactoring how runner process is detected
    • added fallback to cucumber if parallel test was not for parallel formatter
    • reverting some changes
  • launch_uuid related fixes
  • Fix Cucumber parallel formatter

  • Fix parallel_tests version and possible endless loop

  • Add debug logging to debug some issue

  • Add more debug logging

  • Remove retrying of requests to Report Portal

  • Set last_used_time to a time of the parent item

  • Set description to cmd args of parallel_tests process

  • Fix URLs to include required "filter.eq.launch" parameter

  • Fix matcher to detect when start time needs to be updated

  • Clean up formatting in report.rb

  • Implement a folder creation lock file to address duplicate folders being created

  • Delay each process by its ID number to create a staggered start to reporting in Report Portal (addresses load issues)

  • fixing report description when run in parallel

  • if launch_id or file_with_launch_id is provided launch will not be marked as finished
  • moved sleep time, since we are waiting for report to be created anyways
    • reverting file lock changes for reading the file
  • moving lock file related functions to private
    • fixing temp file path generation
  • refactoring how runner process is detected
    • added fallback to cucumber if parallel test was not for parallel formatter
  • using single formatter for both parallel_cucumber and cucumber

  • adding logger for report portal

  • updating readme and adding settings for logging level

  • updating how response processing is handling

  • fixing issue with child process not closing the folder's items

  • fixing value for time, removing cucumber IO as it's no longer needed

    • adding logging
  • when run in parallel, child process should not close items in process(folder)
    • adding logging
  • add exception handling when closing item , if already closed , ignore RP 40018 error code
  • adding patch for multipart

  • updating version

Andrei Botalov and others added 30 commits January 25, 2018 19:28
…porting in Report Portal (addresses load issues)
Continues work and fixes issues with Cucumber parallel executions
@DzmitryHumianiuk
Copy link
Member

@abotalov is it good for merge?
or still require updates?

@abotalov
Copy link
Member

abotalov commented Jul 17, 2019

@DzmitryHumianiuk It still requires updates. Also I've reviewed only some small parts of this PR. I'd prefer if parts I added comments for were addressed before continuing a review.

@DzmitryHumianiuk
Copy link
Member

@abotalov thank you for comment

@DzmitryHumianiuk
Copy link
Member

@abotalov your turn


set_parallel_tests_vars

if ParallelTests.first_process?
Copy link
Member

Choose a reason for hiding this comment

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

No, it's not fixed.
ParallelTests and Sys::ProcTable are not in Gemfile, they are not required dependencies. A user should be able to use the gem without having to install these dependencies.
If user doesn't specify that they want to run tests in parallel, parallel_tests and sys/proctable should not be required and used.

lib/reportportal.rb Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
@@ -54,6 +60,8 @@ Supported settings:
- launch_id - id of previously created launch (to be used if formatter_modes contains attach_to_launch)
- file_with_launch_id - path to file with id of launch (to be used if formatter_modes contains attach_to_launch)
- disable_ssl_verification - set to true to disable SSL verification on connect to ReportPortal (potential security hole!). Set `disable_ssl_verification` to `true` if you see the following error:
- launch_uuid - when formatter_mode is `attach_to_launch`, launch_uuid will be used to create uniq report group(tmp dir should be shared across all lunches)
Copy link
Member

Choose a reason for hiding this comment

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

  • Put a space before the opening parenthesis - report group (tmp dir should be shared across all lunches)
  • lunches -> launches
  • formatter_mode is `attach_to_launch` -> formatter_modes contains `attach_to_launch`

I don't see a difference between launch_id and launch_uuid. launch_id is also a UUID of the launch to be used for attach_to_launch mode. If launch_id is provided by the user, reporting should happen to that launch.

Copy link
Author

Choose a reason for hiding this comment

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

lunch_id is coming from the report portal and forcing you to create lunch beforehand, with luanch_uuid, there is no need to create lunch beforehand. I'm not using the rake and not familiar with it so we have different approaches to solve the same problem.


* With Cucumber and parallel_tests gem:

```parallel_cucumber <some options> -o '<some other options> -f ReportPortal::Cucumber::ParallelFormatter'```
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to have an example for parallel_cucumber to show that the gem supports parallel_tests.

Copy link
Author

Choose a reason for hiding this comment

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

I think you're missing the point, it's single formatted for both

Copy link
Member

@abotalov abotalov Aug 9, 2019

Choose a reason for hiding this comment

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

I understand that it's a single formatter. But the potential user looking at README should also be aware that agent works when tests are run in parallel.

Copy link
Author

Choose a reason for hiding this comment

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

you are more than welcome to contribute.

| attach_to_launch | Do not create a new launch but add executing features/scenarios to an existing launch. Use launch_id or file_with_launch_id settings to configure that. If they are not present client will check rp_launch_id.tmp in `Dir.tmpdir`)
| use_same_thread_for_reporting | Send reporting commands in the same main thread used for running tests. This mode is useful for debugging this Report Portal client. It changes default behavior to send commands in the separate thread. Default behavior is there not to slow test execution. |
| skip_reporting_hierarchy | Do not create items for folders and feature files |
<table><thead><tr><th>Name</th><th>Purpose</th></tr></thead>
Copy link
Member

Choose a reason for hiding this comment

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

This is rendered with horizontal scrolling in Github. So a text becomes harder to read in comparison to the current table rendered using Github flavored markdown.

Copy link
Author

Choose a reason for hiding this comment

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

CapturFiles-20190808_084246

I don't see any scroling

Copy link
Author

Choose a reason for hiding this comment

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

https://github.github.com/gfm/#html-blocks - not sure what you are asking.

Copy link
Member

Choose a reason for hiding this comment

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

As you can see on the screenshot you attached, not all text in column "Purpose" is visible. There is a horizontal scrolling (at least in Chrome).
https://github.com/reportportal/agent-ruby/blob/7be5353c31be361a2a73b93b83f8e4dffacd8c1b/README.md
Screenshot from 2019-08-09 11-50-04

Copy link
Author

Choose a reason for hiding this comment

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

sure, but I don't see what is the issue here. If you know how to do multiple lines in a simple table block let me know.

Copy link
Member

@abotalov abotalov Aug 9, 2019

Choose a reason for hiding this comment

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

Previous formatting looked better in the browser IMO. Could you use it?

Copy link
Author

Choose a reason for hiding this comment

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

That is subjective. Put everything in a single paragraph, and it will not look good in the browser IMO. Hense I had to change it to be more presentable.

@@ -7,12 +7,10 @@ module Cucumber
class Formatter
# @api private
def initialize(config)
ENV['REPORT_PORTAL_USED'] = 'true'

@logger = Logger.new(config.out_stream)
Copy link
Member

Choose a reason for hiding this comment

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

Logger is not loaded by Ruby itself. So there should be a require statement before it's first used.

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

require_relative '../logging/logger'

vveliev-tc and others added 6 commits August 8, 2019 21:15
* Fix sending plain text to attachment

* fixing layout errors

Fix sending plain text to attachment

removing trailing whitespace

* adding gemspec dependency

* updating readme

* fixing gemspec, code formatting

* fixing typo, gemspec update

* fixing issue with formatter when file with launch id is provided

* fixing typos

* fixing typos

* fixing typos
@vveliev-tc
Copy link
Author

It's been a while since PR was open, I have been moved to the different project so if you guys want to merge these changes. PR to my branch is welcome to ensure that it works in our different use cases of the same library. It's been working fine for us so far.

All the checks are passing so I don't see any issues. If you guys want to be picky to the cosmetics of the readme or whatnot, then you can commit changes to make it what you desire. I have no problem using the fork of this project but wanted to contribute back as I like the idea of the report portal. And If you have time for #60(which solves nothing from my opinion), there should not have been a problem finishing this PR as well in a timely fashion.

@vveliev-tc
Copy link
Author

fixed issues that were introduced after rebasing, using persistent connection now

@DzmitryHumianiuk
Copy link
Member

@abotalov are we good to go here?

@abotalov
Copy link
Member

abotalov commented Dec 2, 2019

The scope of this PR is too large. It makes a number of changes in public API, fixes some bugs, adds some features, does a lot of refactoring. So it's hard to review. I'd prefer to just close it.
I'm fine to do sensible changes in PRs with limited scope.

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.

4 participants