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

[CLOSED] Unit test suite for InlineColorEditor #2092

Open
core-ai-bot opened this issue Aug 29, 2021 · 7 comments
Open

[CLOSED] Unit test suite for InlineColorEditor #2092

core-ai-bot opened this issue Aug 29, 2021 · 7 comments

Comments

@core-ai-bot
Copy link
Member

Issue by njx
Wednesday Nov 21, 2012 at 07:17 GMT
Originally opened as adobe/brackets#2174


@jasonsanjose

A few of these unit tests got checked in as part of an earlier pull request (and were reviewed by Peter F), but some of those got modified in this pull anyway, so it's probably worth just reviewing everything from scratch.

Note that these are all mocked tests in one form or another--the first set use a full InlineColorEditor and a mock editor/document (so the InlineColorEditor doesn't actually live in the mock editor); the later ones just test ColorEditor by itself (not inside an InlineColorEditor).

Let me know if you think I should build some more integration-y tests as well. I think these tests give pretty good overall coverage though.


njx included the following code: https://github.com/adobe/brackets/pull/2174/commits

@core-ai-bot
Copy link
Member Author

Comment by njx
Wednesday Nov 21, 2012 at 07:19 GMT


Oops, not quite ready yet...forgot to add the unit tests for the text field.

@core-ai-bot
Copy link
Member Author

Comment by njx
Wednesday Nov 21, 2012 at 07:33 GMT


OK, should be ready for review now.

@core-ai-bot
Copy link
Member Author

Comment by jasonsanjose
Wednesday Nov 21, 2012 at 16:34 GMT


Reviewing

@core-ai-bot
Copy link
Member Author

Comment by jasonsanjose
Wednesday Nov 21, 2012 at 19:27 GMT


Nicely done, especially without firing up a new window. I guess this stuff doesn't rely on ProjectManager, so that's a bonus. Initial review complete.

@core-ai-bot
Copy link
Member Author

Comment by njx
Wednesday Nov 21, 2012 at 20:03 GMT


Changes pushed. I also added JSDoc comments to all the helper functions, since some of their args were just non-obvious enough to require some explanation.

@core-ai-bot
Copy link
Member Author

Comment by njx
Wednesday Nov 21, 2012 at 20:11 GMT


Just added one last little refactoring cleanup I noticed.

@core-ai-bot
Copy link
Member Author

Comment by jasonsanjose
Wednesday Nov 21, 2012 at 20:18 GMT


Looks good. Merging.

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

No branches or pull requests

1 participant