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

Allure reporter add environment variables in config #10452

Merged
merged 29 commits into from Jun 1, 2023

Conversation

m4hdyar
Copy link
Contributor

@m4hdyar m4hdyar commented May 26, 2023

Proposed changes

Closes #10321
With this pull request we can use reportedEnvironmentVars in the config instead of using addEnvironment in tests

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Checklist

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • I have added proper type definitions for new commands (if appropriate)

Further comments

Reviewers: @webdriverio/project-committers

harsha509 and others added 4 commits May 24, 2023 12:19
* update code links for browser->waitUntil

* update example link with SHA commit instead of main
* New translations Donate.md (Bulgarian)

* New translations Element.md (Polish)

* New translations Donate.md (Bulgarian)

* New translations Element.md (Polish)

* New translations Donate.md (Bulgarian)

* New translations Materials.md (Bulgarian)

* New translations code.json (Bulgarian)
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 26, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@christian-bromann
Copy link
Member

christian-bromann commented May 26, 2023

Thanks for raising the pull request.

I am a bit concerned removing addEnvironment entirely because it could potentially break other peoples test. I would recommend better deprecate it and log a warning that the method will be removed soon. Does addEnvironment still has a function in the reporter?

@m4hdyar
Copy link
Contributor Author

m4hdyar commented May 27, 2023

Thanks for raising the pull request.

I am a bit concerned removing addEnvironment entirely because it could potentially break other peoples test. I would recommend better deprecate it and log a warning that the method will be removed soon. Does addEnvironment still has a function in the reporter?

addEnvironment still has a function, but when you use it a second time, it will overwrite the first value. This means you cannot add more than one environment variable. So, if you only need to add one environment variable, it works correctly, although that is not usually the case.
If we keep it, it will also override environment variables that you write in the config.

I understand the importance of not disrupting people's tests. But the error is relatively easy to fix. However, in my opinion, deprecating addEnvironment without correcting its functionality would result in incorrect functionality without displaying any error, leading to unpredictable behavior for users.

On the other hand, if we choose to deprecate and also correct the behavior of addEnvironment, it would involve introducing a significant amount of unnecessary complexity.

What do you think?

dependabot bot added 9 commits May 29, 2023 12:23
Bumps [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/parser) from 5.59.6 to 5.59.7.
- [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases)
- [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/parser/CHANGELOG.md)
- [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v5.59.7/packages/parser)

---
updated-dependencies:
- dependency-name: "@typescript-eslint/parser"
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [@typescript-eslint/utils](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/utils) from 5.59.6 to 5.59.7.
- [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases)
- [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/utils/CHANGELOG.md)
- [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v5.59.7/packages/utils)

---
updated-dependencies:
- dependency-name: "@typescript-eslint/utils"
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…0458)

Bumps [devtools-protocol](https://github.com/ChromeDevTools/devtools-protocol) from 0.0.1146845 to 0.0.1149535.
- [Changelog](https://github.com/ChromeDevTools/devtools-protocol/blob/master/changelog.md)
- [Commits](ChromeDevTools/devtools-protocol@v0.0.1146845...v0.0.1149535)

---
updated-dependencies:
- dependency-name: devtools-protocol
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [aws-sdk](https://github.com/aws/aws-sdk-js) from 2.1381.0 to 2.1386.0.
- [Release notes](https://github.com/aws/aws-sdk-js/releases)
- [Commits](aws/aws-sdk-js@v2.1381.0...v2.1386.0)

---
updated-dependencies:
- dependency-name: aws-sdk
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [@types/jasmine](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/jasmine) from 4.3.1 to 4.3.2.
- [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases)
- [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/jasmine)

---
updated-dependencies:
- dependency-name: "@types/jasmine"
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
)

Bumps [@sveltejs/vite-plugin-svelte](https://github.com/sveltejs/vite-plugin-svelte/tree/HEAD/packages/vite-plugin-svelte) from 2.3.0 to 2.4.1.
- [Release notes](https://github.com/sveltejs/vite-plugin-svelte/releases)
- [Changelog](https://github.com/sveltejs/vite-plugin-svelte/blob/main/packages/vite-plugin-svelte/CHANGELOG.md)
- [Commits](https://github.com/sveltejs/vite-plugin-svelte/commits/@sveltejs/vite-plugin-svelte@2.4.1/packages/vite-plugin-svelte)

---
updated-dependencies:
- dependency-name: "@sveltejs/vite-plugin-svelte"
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [@vitest/coverage-c8](https://github.com/vitest-dev/vitest/tree/HEAD/packages/coverage-c8) from 0.30.1 to 0.31.1.
- [Release notes](https://github.com/vitest-dev/vitest/releases)
- [Commits](https://github.com/vitest-dev/vitest/commits/v0.31.1/packages/coverage-c8)

---
updated-dependencies:
- dependency-name: "@vitest/coverage-c8"
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [strip-ansi](https://github.com/chalk/strip-ansi) from 6.0.1 to 7.1.0.
- [Release notes](https://github.com/chalk/strip-ansi/releases)
- [Commits](chalk/strip-ansi@v6.0.1...v7.1.0)

---
updated-dependencies:
- dependency-name: strip-ansi
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [dependabot/fetch-metadata](https://github.com/dependabot/fetch-metadata) from 1.4.0 to 1.5.1.
- [Release notes](https://github.com/dependabot/fetch-metadata/releases)
- [Commits](dependabot/fetch-metadata@v1.4.0...v1.5.1)

---
updated-dependencies:
- dependency-name: dependabot/fetch-metadata
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@christian-bromann
Copy link
Member

However, in my opinion, deprecating addEnvironment without correcting its functionality would result in incorrect functionality without displaying any error, leading to unpredictable behavior for users.

Why we can't deprecated and notify people at the same time about the error, even give them some instructions how to use the new interface. I am not sure why keeping addEnvironment imposes more complexity but I would rather want to keep it with a deprecation warning.

udarrr and others added 3 commits May 30, 2023 17:54
…ebdriverio#10454)

* added new service have been named 'wdio-robonut-service'

* fixed title mistake

* added wdio-robonut-service to cli constants external section
* New translations options.json (Bulgarian)

* New translations current.json (Bulgarian)

* New translations current.json (Bulgarian)

* New translations current.json (Bulgarian)

* New translations code.json (Bulgarian)

* New translations current.json (Bulgarian)

* New translations current.json (Bulgarian)

* New translations footer.json (Bulgarian)

* New translations navbar.json (Bulgarian)

* New translations code.json (Bulgarian)

* New translations current.json (Bulgarian)

* New translations Browser.md (Bulgarian)

* New translations Team.md (Bulgarian)

* New translations code.json (Bulgarian)

* New translations code.json (Bulgarian)

* New translations Environment.md (Polish)

* New translations Environment.md (Polish)

* New translations Environment.md (Polish)

* New translations Browser.md (Polish)

* New translations Globals.md (Polish)

* New translations Mock.md (Polish)

* New translations Mock.md (Polish)
Co-authored-by: Luke Fitzgerald <luke.fitzgerald@nutrien.com>
@BorisOsipov
Copy link
Member

BorisOsipov commented May 30, 2023

@m4hdyar @christian-bromann

The best solution for me would be to deprecate addEnvironment and display a warning message with a link to the documentation. Additionally, we should modify addEnvironment to have no functionality at all. This approach will alert users to switch to the new method(config approach) and provide an easy way to migrate. Of course, we may encounter some issues where users complain that addEnvironment is not working, but there is no better solution available.

@christian-bromann
Copy link
Member

Sounds reasonable @BorisOsipov 👍

christian-bromann and others added 5 commits May 31, 2023 13:17
* New translations Modules.md (Polish)

* New translations Modules.md (Polish)
Bumps [got](https://github.com/sindresorhus/got) from 12.6.1 to 13.0.0.
- [Release notes](https://github.com/sindresorhus/got/releases)
- [Commits](sindresorhus/got@v12.6.1...v13.0.0)

---
updated-dependencies:
- dependency-name: got
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@m4hdyar
Copy link
Contributor Author

m4hdyar commented Jun 1, 2023

@christian-bromann Sounds reasonable to me too.
I've added it again as a deprecated function.

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@BorisOsipov any comments?

packages/wdio-allure-reporter/src/common/api.ts Outdated Show resolved Hide resolved
@BorisOsipov
Copy link
Member

LGTM

@christian-bromann
Copy link
Member

christian-bromann commented Jun 1, 2023

@m4hdyar mind signing the CLA, also please see my comment?

Change the deprecation message

Co-authored-by: Christian Bromann <git@bromann.dev>
@m4hdyar
Copy link
Contributor Author

m4hdyar commented Jun 1, 2023

@christian-bromann In order to sign EasyCLA, I had to change the commit's author and email so I did a rebase. If this is not suitable I can make another pull request with the appropriate commits.

@christian-bromann
Copy link
Member

That should work, thanks!

@christian-bromann christian-bromann added the PR: Bug Fix 🐛 PRs that contain bug fixes label Jun 1, 2023
@christian-bromann christian-bromann merged commit 7d4f930 into webdriverio:main Jun 1, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Bug Fix 🐛 PRs that contain bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: Allure reporter adding environment value overwrites the last value
9 participants