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

Tests utils should be moved from tests to src #333

Closed
pjasiun opened this issue Sep 21, 2016 · 8 comments
Closed

Tests utils should be moved from tests to src #333

pjasiun opened this issue Sep 21, 2016 · 8 comments
Labels
type:task This issue reports a chore (non-production change) and other types of "todos".

Comments

@pjasiun
Copy link

pjasiun commented Sep 21, 2016

And there are 3 good reasons to do it.

  • https://github.com/ckeditor/ckeditor5-dev-compiler/issues/15 For the third-party developer, tests are not important, there should be an option to exclude tests in the compiler. At the same time, one may want to have testing tools for his tests.
  • Event if tests are not excluded, it is not possible to use testing tools because they have absolute paths which may not work with the extended building environment (for instance they does not work with Webpack, because they starts from /, and Webpack does not expect / at the beginning of the absolute path).
  • In fact, I do not see any good reason why not to use some of them in the code, at least for debugging.
@pjasiun pjasiun added ★★★ type:task This issue reports a chore (non-production change) and other types of "todos". labels Sep 21, 2016
@Reinmar
Copy link
Member

Reinmar commented Sep 21, 2016

I'm not sure I like the idea of moving them to src/. We must make them usable, but I'm not entirely sure that src/ is the right place for them. I'm afraid that what lands there becomes public. We just recently moved some tools from src/ to tests/ exactly because of that.

So I'd think on how we can make them accessible instead of just moving them. Perhaps we'll be able to find a better solution. If not, we'll have to document this clearly.

BTW. I also wanted to use those tools in a build, so I agree that this is quite important :)

@pjasiun
Copy link
Author

pjasiun commented Sep 22, 2016

I do not insist then need to be in the src, but they need to have relative paths to make then usable.

@pjasiun
Copy link
Author

pjasiun commented Sep 22, 2016

Otherwise one need to create a separate process to transform or handle these absolute paths only to handle testing tools.

@scofalik
Copy link
Contributor

I am a bit torn. On one hand I feel like I don't really have any problem with moving this to src. On the other hand, as PK said, it will be public then, it would need to be documented, we will need to keep API, etc.

AFAI understand how builder works, files must be either in tests or src directory. No place for tools or something there. So IDK how to solve this really. I'd like to avoid making those tools public so people won't run into problems using them.

@scofalik
Copy link
Contributor

We could move it to src/tools and state in documentation that these are testing tools that should not be used in non-test code buuut... if people start using those tools we need to keep and maintain them in the same way we do with "normal" code, so there's not difference really.

@pjasiun
Copy link
Author

pjasiun commented Sep 22, 2016

I am a bit torn. On one hand I feel like I don't really have any problem with moving this to src. On the other hand, as PK said, it will be public then, it would need to be documented, we will need to keep API, etc.

If we want to let others use these tools we need to add the documentation to them and keep API anyway.

And, I think that it is important anyway to have a good documentation for these tools, because otherwise, they are useful only for people who know them.

@Reinmar
Copy link
Member

Reinmar commented Sep 22, 2016

I agree that those tools should be documented and we've got that documentation already. We just don't know where to put it in the namespaced API docs, but this will be solved when we switch to modules.

As for how to make them accessible... We'll be considering switching to Webpack+Karma soon, so perhaps we can postpone this topic until then. The way how tests import things from the rest of the code will change and perhaps we'll see then how to expose the test utils.

OTOH, I think I can live with putting them inside src/ and documenting them as "test utils; subject to frequent changes; not part of the public API (semver doesn't apply)". However, not all test utils will make sense there... There may be some which are strictly related to testing. So perhaps this is case by case.

@Reinmar
Copy link
Member

Reinmar commented Sep 22, 2016

I reported a specific ticket for exposing engine utils: https://github.com/ckeditor/ckeditor5-engine/issues/610. I also explained there that we want to differentiate between dev utils and test utils. The former can be exposed, the latter not. I guess that the difference between them is quite clear too. Dev utils can be useful during development, tests utils are useful for testing.

Since there were no other dev utils mentioned in this ticket, I will close it. If any other dev utils should be exposed as well, a specific ticket in a specific repository should be created.

@Reinmar Reinmar closed this as completed Sep 22, 2016
mlewand pushed a commit that referenced this issue May 1, 2020
Other: Removed `env.isEdge` as Edge is now detected and treated as Chrome. Closes #6202.

BREAKING CHANGE: `env.isEdge` is no longer available. See #6202.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

No branches or pull requests

3 participants