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

[CSR-2066] feat: added full test suite generation for convert command #139

Merged
merged 17 commits into from
Feb 10, 2025

Conversation

miguelangaranocurrents
Copy link
Contributor

@miguelangaranocurrents miguelangaranocurrents commented Jan 24, 2025

Description

Provide a brief description of the changes in this PR. Important to list all the changes made in overall. Describe any improvements, follow up tasks or edge cases related to this PR

This PR add the fullTestSuite.json file generation to the convert command.
It also allows generating multiple groups for the full test suite by accepting multiple files as different test groups.

PR Checklist

  • I performed manual tests or added tests that prove my fix is effective or that my feature works.
  • I have performed a self-review of my code.
  • I have annotated my PR with comments, particularly in hard-to-understand areas.
  • I have considered the security implications of this work.

Release Plan

Do we need to update any environment variables? Is there any order of releases required?

  • Package release

Demo

Add screenshots or videos demonstrating the solution if applicable

https://www.loom.com/share/02ef05e48df64832a7cb884c960db874?sid=60ea25b0-e032-4e73-b888-1d0b0dffe6c3

Manual Testing

Describe in steps (support it with images if needed) how to try the changes of this PR. (Even the start/setup of the services needed to try it out). If it's a bug also include reproduction steps

  1. Use the convert command to create instance files and the fullTestSuite JSON file
  2. Use multiple JUnit XML files for having multiple groups
  3. Use the upload command

Copy link

@miguelangaranocurrents miguelangaranocurrents requested review from twk3 and vCaisim and removed request for twk3 February 5, 2025 16:29
@miguelangaranocurrents miguelangaranocurrents force-pushed the feat/migrate-fulltestsuite-convert branch from c9d00d8 to ee4ec7e Compare February 5, 2025 17:18
Copy link
Collaborator

@vCaisim vCaisim left a comment

Choose a reason for hiding this comment

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

Hey Miguel, there is a little more work to be done here.

While testing I noticed a bug: if the groups contain the same spec names, the new created instances will overwrite the old ones and only the last group instances will be in the report directory. Check the video

Screencast.from.2025-02-06.10-42-17.mp4

packages/cmd/src/services/convert/getParsedXMLInput.ts Outdated Show resolved Hide resolved
packages/cmd/src/services/convert/getParsedXMLInput.ts Outdated Show resolved Hide resolved
packages/cmd/src/services/convert/getParsedXMLInput.ts Outdated Show resolved Hide resolved
packages/cmd/src/services/convert/getParsedXMLInput.ts Outdated Show resolved Hide resolved
packages/cmd/src/services/convert/getParsedXMLInput.ts Outdated Show resolved Hide resolved
packages/cmd/src/services/convert/index.ts Outdated Show resolved Hide resolved
packages/cmd/src/lib/fs.ts Outdated Show resolved Hide resolved
packages/cmd/src/commands/convert/options.ts Outdated Show resolved Hide resolved
examples/jest/src/basic/basic.spec.ts Outdated Show resolved Hide resolved
@miguelangaranocurrents miguelangaranocurrents force-pushed the feat/migrate-fulltestsuite-convert branch from ee4ec7e to eea7b2f Compare February 6, 2025 16:04
@miguelangaranocurrents
Copy link
Contributor Author

@vCaisim ready now, I fixed the bug of the same name instances being overwritten.

Copy link
Collaborator

@vCaisim vCaisim left a comment

Choose a reason for hiding this comment

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

Tested, the issue is solved. Please address the latest feedback and update the option description for the --input-file so it is clear that it can accept multiple paths

packages/cmd/src/lib/fs.ts Outdated Show resolved Hide resolved
packages/cmd/src/services/convert/getFullTestSuite.ts Outdated Show resolved Hide resolved
return parsedXMLInputs;
}

const getParsedXMLInput = async (XMLString: string) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • getParsedXMLInput, don't trim 2 times, use a variable
  • it returns undefined if XMLString is emtpy string or parsedXMLInput is falsy value and getParsedXMLArray ends up with [undefined]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first point is done.
The second point:
I'm applying a .filter(Boolean); to the array after it is finished to remove the possible undefined elements. This is in line 14 of the same file.

@miguelangaranocurrents
Copy link
Contributor Author

--input-file

Thanks for the feedback. Everything is addressed now :)

@vCaisim vCaisim merged commit 03e06cb into main Feb 10, 2025
4 checks passed
@vCaisim vCaisim deleted the feat/migrate-fulltestsuite-convert branch February 10, 2025 14:24
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