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

e2e-test: remove test/automation #5882

Merged
merged 20 commits into from
Jan 7, 2025
Merged

e2e-test: remove test/automation #5882

merged 20 commits into from
Jan 7, 2025

Conversation

midleman
Copy link
Contributor

@midleman midleman commented Jan 6, 2025

To centralize all relevant e2e test code and enhance clarity, the e2e test structure has been reorganized:

  • Removed the positron prefix from all of our POMs, as all Microsoft POMs have been removed so this is no longer needed.
  • Moved (most of) test/automation to test/e2e/infra.
    • The one exception was moving test/automation/src/positron to test/e2e/pages
  • Renamed test/e2e/areas to test/e2e/tests
  • Updated watcher, only need to watch test/e2e now

This is the new e2e dir structure:

test/
└── e2e/
    ├── infra/   <-- contains all the driver, browser, electron, etc files
    ├── pages/   <-- contains all the Positron POMs
    └── tests/   <-- duh. the tests
    
 note: test/automation and test/smoke no longer exist!!

FYI
Since the driver files have been moved into our own directory, I’ve removed all “Start/End Positron” comment snippets. There’s no need to worry about upstream merges anymore, as all of our code now resides within our own directory.

QA Notes

Ran Full Suite
Ran Tests against Release

In next PR, will update the README

Copy link

github-actions bot commented Jan 6, 2025

E2E Tests 🚀  ?
This PR will run tests tagged with: @critical

@midleman midleman changed the title e2e-test: remove positron prefix from POMs e2e-test: remove test/automation Jan 6, 2025
@midleman midleman marked this pull request as ready for review January 6, 2025 23:02
jonvanausdeln
jonvanausdeln previously approved these changes Jan 6, 2025
Copy link
Contributor

@jonvanausdeln jonvanausdeln left a comment

Choose a reason for hiding this comment

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

LGTM... with a couple of comments...

  • I'm on the fence about removing positron from POM, etc. I kind of like having the moniker to make clear this is not part of VSCode, but I get that it's not relevant any longer.
  • Will the removal of test\smoke and test\automation cause any issues with upstream merge?

@midleman
Copy link
Contributor Author

midleman commented Jan 7, 2025

LGTM... with a couple of comments...

  • I'm on the fence about removing positron from POM, etc. I kind of like having the moniker to make clear this is not part of VSCode, but I get that it's not relevant any longer.
  • Will the removal of test\smoke and test\automation cause any issues with upstream merge?

Thanks for the feedback! I totally get where you’re coming from about the “Positron” prefix - it can be helpful for clarity. However, I’d like to share why I think removing it from class and POM names might be beneficial overall:

  1. Dropping the prefix makes the class and POM names shorter and easier to read, especially when they’re referenced frequently throughout the code. A more succinct structure also aligns with standard naming conventions.
  2. Since these files already live in a directory exclusive to Positron code, the context is clear. The directory itself acts as a natural “namespace,” so prefixing the names becomes redundant.
  3. Long, prefixed names can clutter IDE suggestions and make it harder to find what you need quickly. Shorter, more descriptive names keep autocomplete lists cleaner and easier to scan, which speeds up development and reduces the chance of selecting the wrong class or method.

As for the upstream merge, since our files have moved to a completely separate directory, they no longer overlap with the upstream structure. Because of this, there’s no risk of merge conflicts or accidental overwrites - we’re fully decoupled from the upstream code.

Does that sound reasonable? Happy to hear your thoughts!

testlabauto
testlabauto previously approved these changes Jan 7, 2025
Copy link
Contributor

@testlabauto testlabauto left a comment

Choose a reason for hiding this comment

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

LGTM!

Not really sure we should have changed the license note on those files, but its probably fine.

@midleman
Copy link
Contributor Author

midleman commented Jan 7, 2025

LGTM!

Not really sure we should have changed the license note on those files, but its probably fine.

I wasn't totally sure either... should I change it back?

@testlabauto
Copy link
Contributor

LGTM!
Not really sure we should have changed the license note on those files, but its probably fine.

I wasn't totally sure either... should I change it back?

I think the policy has been to leave the existing license on any file inherited from Microsoft and put our license on new files. Moving a file is kind of a special case, but I'd lean towards leaving theirs on them.

Copy link
Contributor

@testlabauto testlabauto left a comment

Choose a reason for hiding this comment

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

LGTM!

@midleman midleman merged commit be25a0f into main Jan 7, 2025
6 checks passed
@midleman midleman deleted the mi/rename-poms branch January 7, 2025 15:50
@github-actions github-actions bot locked and limited conversation to collaborators Jan 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants