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

Add support for influencing history id based on environment properties #494

Closed
adfoster-r7 opened this issue May 29, 2023 · 8 comments · Fixed by #498
Closed

Add support for influencing history id based on environment properties #494

adfoster-r7 opened this issue May 29, 2023 · 8 comments · Fixed by #498
Labels
bug Something isn't working

Comments

@adfoster-r7
Copy link

Scenario:

When running the same test suite against in parallel against different host operating systems (i.e. mac/linux/windows), I noticed that the some test results were missing from the generated suites page after generating the allure report - but the test runs were available in the timeline view

I tracked this down to the results historyId being re-used multiple times across my runners:

{
  // ...
  "parameters": [
    {
      "name": "environment",
      "value": "darwin19"
    }
  ],
  "uuid": "6c9a7870-dff7-013b-1c3f-005056a70044",
  "historyId": "5fadf4ef80bc505e534df630bc674529",
  // ...
}

Digging deeper, I noticed that the value being generated is based on the example.id, i.e. "./spec/acceptance/foo_spec.rb[1:2:1:1:11:1] - which won't be unique when running the test suite on different github action runners in parallel against windows/linux/macos

It would be great if we could influence the history id in some capacity, so that the environment properties could be considered too

@andrcuns andrcuns added bug Something isn't working and removed bug Something isn't working labels May 30, 2023
@andrcuns
Copy link
Collaborator

It is actually influenced by environment. It happens when generating the unique id string based on the passed example id: https://github.com/allure-framework/allure-ruby/blob/master/allure-ruby-commons/lib/allure_ruby_commons/model/test_result.rb#L27

So the problem must be something else.

@adfoster-r7
Copy link
Author

Thanks for taking a look 👍

With fresh eyes; I believe the current implementation is honoring allure_config.environment, but not taking into consideration the values in allure_config.environment_properties

For context I believe this was the example I originally followed from the docs:

## Configuration
Following configuration options are supported:
```ruby
AllureRspec.configure do |config|
config.results_directory = "report/allure-results"
config.clean_results_directory = true
config.logging_level = Logger::INFO
config.logger = Logger.new($stdout, Logger::DEBUG)
config.environment = "staging"
# these are used for creating links to bugs or test cases where {} is replaced with keys of relevant items
config.link_tms_pattern = "http://www.jira.com/browse/{}"
config.link_issue_pattern = "http://www.jira.com/browse/{}"
# additional metadata
# environment.properties
config.environment_properties = {
custom_attribute: "foo"
}
# categories.json
config.categories = File.new("my_custom_categories.json")
end
```

Current values:

[3] pry(#<AllureRspec::RSpecFormatter>)> allure_config.environment
=> "darwin20"
[4] pry(#<AllureRspec::RSpecFormatter>)> allure_config.environment_properties
=> {:host_os=>"darwin20", :ruby_version=>"3.0.5"}

I would have expected the allure_config.environment_properties to influence the history id.
I think this also explains why my properties weren't appearing here in the Allure report either:

image

Let me know if this is user error, or something that needs to be tweaked 👍

@andrcuns
Copy link
Collaborator

environment.properties file indeed is not taken in consideration because it's not really test related thing from report perspective, it's what generates the Environment section on the main page, like so:
image

The parameter environment in the test itself however is slightly different, it can only be a single value and there can't be several environment properties. It's main purpose is to differentiate same specs running on different environments so they are not dropped in to retries.

But I can see how you could have multi-dimensional run matrix, so one parameter isn't enough, maybe there is a better way, not 100% sure.

@andrcuns
Copy link
Collaborator

cc: @baev

I have not been following super closely how other adapters implement similar functionality lately, maybe there is something to improve here?

@adfoster-r7
Copy link
Author

adfoster-r7 commented May 31, 2023

I can see how you could have multi-dimensional run matrix, so one parameter isn't enough

That's my exact scenario 👍 I'm testing the same library code by running the same test suite against different operating systems and ruby combinations with Github action's matrix support

@andrcuns
Copy link
Collaborator

andrcuns commented Jun 1, 2023

The main problem is that environment.properties can have data that should not be taken in to account when generating history id, like app versions, revisions etc and there is no real way to know that.

The default environment functionality just uses built in allure parameter functionality to create a parameter named environment. But You can also just set those parameters yourself via parameter helper method.

before do |example|
  example.parameter("ruby", "3.1")
  example.parameter("os", "darwin20")
end

Full list of parameters is still not taken in to account when generating history_id which actually is a bug now that I think about it.

@adfoster-r7
Copy link
Author

Thanks for pointing that API out, my parameters are now correctly tracked in the UI now - but the history id still isn't changed based on those values as you say 👍

@andrcuns
Copy link
Collaborator

andrcuns commented Jun 2, 2023

One more solution that might be useful, is to allow passing a more complex json string in to allure_config.environment option and parse it.

Or rather it can be hash if it's passed programmatically, or json string if passed via environment variable. It would allow a bit more flexibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants