-
Notifications
You must be signed in to change notification settings - Fork 43
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
Get tests to work #43
base: master
Are you sure you want to change the base?
Conversation
@Jules-Bertholet Have you seen ckeditor/ckeditor5-dev#594? Do you think it'd impact this? Bikeshed: Do you have any thoughts if that'd impact the need to include a submodule dependency? I'm interested in avoiding that if possible. I suppose another option could be to clone the ckeditor5 repo via a script and have it under |
No, ckeditor/ckeditor5-dev#594 as is wouldn't eliminate the need for getting files from the main repo (note that all that is needed is the contents of https://github.com/ckeditor/ckeditor5/tree/master/packages/ckeditor5-core/tests/_utils). |
@Jules-Bertholet Any comments on using a shell script that'd check if it exists and clone it rather than a submodule? The reason I ask is I think submodules are heavy. Development and building doesn't need them. If they're only part of tests, we may be able to avoid attaching the repo a the git level. Also we may be able to use commands that clone with less depth ( If you'd like I can propose something |
Go ahead |
@Jules-Bertholet Absolutely Note: I may not be able to get around to that immediately. I'm assuming the "allow contributions from maintainers" is checked in the right side of the PR though? |
It is checked, yes |
@Jules-Bertholet Could you rebase this PR? (to fix the README conflict?) I reformatted the README with prettier in fef1e16 Could you take a stab at this w/o the git submodule? e.g. creating a shell script + npm The reason why is git submodules are heavy duty for a one-off case and it breaks from the pattern of using npm packages. |
This commit introduces a Git submodule to the repo
I've updated the instructions for fetching the submodule so that only needed files are downoaded (this makes the submodule solution much lighter). |
@tony I've added a |
Note that once ckeditor/ckeditor5#7311 is merged we will be able to dump the submodule completely. |
One detail - I think we should avoid the git submodule part unless we've no other option. Do you use them often? I do but want to try without it for this case. The reason why is this is only used for tests and I think submodules would be for cases where we fundamentally relied on it (even for normal dev/packaging, but we use NPM for that). If it's true ckeditor/ckeditor5#7311 is near, that is something I'd like to explore since it fits the packaging setup here better. Have you considered |
I've eliminated the submodules |
Thank you!!! The
This was due to my git version: I updated to |
When running locally, here is what I get:
|
@Jules-Bertholet Would you like to take a look at this and see if you get the same error when trying again? (e.g. If not, I can merge this in and try to fix after. My plan is to set up a GH action to run these |
Should work now |
@Jules-Bertholet I think this needs to pull the correct tag from ckeditor5 Do tests pass for you with Even with
|
No, tests don't pass for me. I only intended to get them to run, never look at why they were failing |
@@ -0,0 +1,2 @@ | |||
{ | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Jules-Bertholet Is this needed? We could also at .vscode/
, .vim/
to .gitignore
Roger that! That's still helpful! |
Tests currently do not work see - isaul32#43 (comment)
Closes #36. Unfortunately this required adding the main ckeditor5 repo as a submodule, you will have to run
git submdule init
andgit submodule update
.To actually run the tests, use
yarn test
, optionally with--coverage
,--watch
, or--source-map
.