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

Cover all Editor implementations with tests #156

Closed
nicoespeon opened this issue Sep 5, 2020 · 4 comments
Closed

Cover all Editor implementations with tests #156

nicoespeon opened this issue Sep 5, 2020 · 4 comments
Assignees
Labels
📦 Refactor Change that doesn't modify existing behavior

Comments

@nicoespeon
Copy link
Owner

nicoespeon commented Sep 5, 2020

I had decided not to write integration tests because I couldn't make Jest work with vscode-api.

It means that changes made to the VSCodeEditor implementation should be manually tested. It's OK because it doesn't change very often.

But as I'm implemented a VueVSCodeEditor now, I have the same problem. I've been thinking about it, and I have 2 solutions in mind:

  1. Try again to get Jest & vscode-api work together. Last try was in May 2019 so things might have improved here.
  2. Otherwise, wrap vscode-api in a Facade that I can inject to VSCode editors. I will be able to Subclass & Override it in tests, so I could apply the existing contract tests to VSCodeEditor and VueVSCodeEditor. This will bring more code in the "Domain" part of Abracadabra and leave very simple code in the Infrastructure part, which is desirable if I can't test my Editor implementations in integration.

I finally managed to get Jest & mocha working together, so no need for both scenario described above.

What would scenario 2 have looked like?

What we have now

image

What we will have after

image

class VSCodeEditor implements Editor {
  constructor(private vscodeApi: VSCodeAPI) {}
}
class VSCodeAPI {
  // Thin Facade on top of `vscode-api`
  // It just makes it easier to Subclass in tests (no need to hijack the import mechanism to mock)
}

And in tests:

class FakeVSCodeAPI extends VSCodeAPI {
  // Override methods so we can control read/write
  // Make it possible to test the logic of VSCodeEditor
  // VSCodeAPI logic will be left untested, until we can do integration tests
}

If we can do integration tests, this would still be interesting in making vscode-api upgrades easier. But not by much. It would be equally fine to test VSCodeEditor in integration because it's still small.

@nicoespeon nicoespeon added the 💪 Improvement Refactoring doesn't run on a specific case or result could be optimized label Sep 5, 2020
@nicoespeon nicoespeon added 📦 Refactor Change that doesn't modify existing behavior and removed 💪 Improvement Refactoring doesn't run on a specific case or result could be optimized labels Dec 10, 2020
@nicoespeon nicoespeon self-assigned this Jan 10, 2021
@nicoespeon
Copy link
Owner Author

Doing great, promising progress:

image

I could access VS Code API from within a test file, using mocha, without having conflicts with Jest unit tests that still exist in the project.

I need to polish the configuration & make everything easy to use, but I should have that ready very soon 🎉

@nicoespeon
Copy link
Owner Author

nicoespeon commented Jan 21, 2021

Got them running! Remaining tasks before calling it a day:

  • Document how it works
  • Get them run in CI (if possible, not sure about VS Code limitations about that)

@nicoespeon
Copy link
Owner Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 Refactor Change that doesn't modify existing behavior
Projects
None yet
Development

No branches or pull requests

1 participant