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

Make it possible to create absolute path based on basePath #12

Closed
pjasiun opened this Issue May 23, 2014 · 19 comments

Comments

Projects
None yet
3 participants
@pjasiun
Copy link
Contributor

pjasiun commented May 23, 2014

Because in the bender.js every test have a defined basePath it would be good the be able to create absolute paths based on basePath using some kind of macro (eg.: BASE_PATH/assets/lib.js), in both js and html files.

@gregpabian

This comment has been minimized.

Copy link
Collaborator

gregpabian commented May 26, 2014

Alright, I'll add %BASE_PATH% markup to use in tests and html templates.

@Reinmar

This comment has been minimized.

Copy link
Contributor

Reinmar commented Jun 3, 2014

After 918076c this become critical. Actually... we have to review many tests again :(

@Reinmar

This comment has been minimized.

Copy link
Contributor

Reinmar commented Jun 3, 2014

Q: what do we really need? We know the CKEditor base path thanks to CKEDITOR.basePath. What we don't know is a path to assets directories (the main one and the test's one - so basically the test base path).

I just realised that this may be harder than we thought. Because of #27 the path may change depending on group of tests unless in #27 we assumed that adding a test group which is kept outside BENDER_ROOT/tests/ will not modify paths to other tests.

Note - I want (I believe that we must) achieve a situation in which tests don't have to reviewed after moving them, adding other test groups, changes in the way how Bender generates paths, etc.

Grzesiek - could you comment on that? I think we need more details about how tests paths are created and what happens with them depending on defined test groups. I'm very interested in case when we'll start keeping tests in plugins directories.

@pjasiun

This comment has been minimized.

Copy link
Contributor Author

pjasiun commented Jun 3, 2014

The problem with CKEDITOR.basePath is that it is JS and in most cases we want to use absolute path in HTMLs head. Also it is Editors base path and we need tests base path. We should not build tests base path based on the editors base path.

@Reinmar

This comment has been minimized.

Copy link
Contributor

Reinmar commented Jun 3, 2014

Have you got an example of test in which you need to access CKEditor files (only then you need to know its path) from the head of your test?

And even if you need to do that, then you can do that from JS because there you can access CKEDITOR.basePath. So the problem isn't CKEditor's path.

The problem is assets path.

@gregpabian

This comment has been minimized.

Copy link
Collaborator

gregpabian commented Jun 3, 2014

OK, so we have to think about 2 scenarios:

  • we run a test from the dashboard - no matter if by opening a link or by pressing "Run tests" button. In this scenario a test URL represents the actual file system structure relative to cwd:
    <SERVER_URL>:<SERVER_PORT>/<BASE_PATH>/<TEST_PATH>/</TEST_FILE>

    And, since we know our tests directory structure we can easily use relative paths to assets and everything should be fine.

  • we run a test within a job - in this case, a test URL is a bit different:
    <SERVER_URL>:<SERVER_PORT>/jobs/<JOB_ID>/tests/<BASE_PATH>/<TEST_PATH>/</TEST_FILE>

    (BTW I could think of removing this tests in the middle, but I'm not sure if it's really necessary?)

    The above URL represents the actual file system structure relative to .bender/ directory inside of the project.

@Reinmar

This comment has been minimized.

Copy link
Contributor

Reinmar commented Jun 3, 2014

<SERVER_URL>:<SERVER_PORT>/<BASE_PATH>/<TEST_PATH>/</TEST_FILE>

Didn't #27 conditionally remove <BASE_PATH> from this?

@gregpabian

This comment has been minimized.

Copy link
Collaborator

gregpabian commented Jun 3, 2014

No, first you probably mean #26, second the old URL was:
<SERVER_URL>:<SERVER_PORT>/tests/<BASE_PATH>/<TEST_PATH>/</TEST_FILE>
so I didn't remove the base path but /tests/ part. I wouldn't remove BASE_PATH at all - it won't give us anything but a lot of headache...

@pjasiun

This comment has been minimized.

Copy link
Contributor Author

pjasiun commented Jun 3, 2014

@Reinmar Paths like this should be IMHO absolute, not related:
https://github.com/ckeditor/ckeditor-dev/blob/master/tests/tickets/9456/1.html#L2

And based on the main tests folder, not on the CKEditor basePath.

@Reinmar

This comment has been minimized.

Copy link
Contributor

Reinmar commented Jun 3, 2014

@pjasiun This is not a path to CKEditor directory!! This a path to assets directory in the tests.

@gregpabian

This comment has been minimized.

Copy link
Collaborator

gregpabian commented Jun 3, 2014

The only argument behind using absolute paths is when you start messing with test folder structure - moving some tests to subdirectories etc. it may still work, but you can break something...

@pjasiun

This comment has been minimized.

Copy link
Contributor Author

pjasiun commented Jun 3, 2014

@Reinmar This is the type of the paths I would like to change using BASE_PATH. I never said I need a base path to the CKEditor.

@gregpabian Exactly. I think that related path between tests is a bad idea, especially if we want to move them to plugins one day.

@Reinmar

This comment has been minimized.

Copy link
Contributor

Reinmar commented Jun 3, 2014

The only argument behind using absolute paths is when you start messing with test folder structure - moving some tests to subdirectories etc. it may still work, but you can break something...

Unfortunately there's one more argument - when an asset is kept in test directory and we use CKEditor API to load it (e.g. we're loading a styles set) we need a path relative to CKEditor.basePath... Am I right?

@gregpabian

This comment has been minimized.

Copy link
Collaborator

gregpabian commented Jun 3, 2014

Alright so I'm gonna implement it to point to current test group's base path. Just a quick question - should it contain a slash a the end or not?

@Reinmar

This comment has been minimized.

Copy link
Contributor

Reinmar commented Jun 3, 2014

Alright so I'm gonna implement it to point to current test group's base path. Just a quick question - should it contain a slash a the end or not?

I just realised that it's enough. Main assets directory is kept outside of test groups directories. And making a path to this directory from e.g. a test in /plugins/image2/tests/foo.js would be complicated.

So I think that there must be defined the tests root - in this case it will point to /tests/ and it would be a setting specific for a project (so bender.js it is). Second necessary path is a path to test file's directory, because an asset can be stored there.

@pjasiun

This comment has been minimized.

Copy link
Contributor Author

pjasiun commented Jun 3, 2014

@gregpabian I'm for slash at the end, so BASE_PATH would be "...tests/".

@gregpabian

This comment has been minimized.

Copy link
Collaborator

gregpabian commented Jun 3, 2014

Main assets directory is inside of every group's basePath in our case so which part is complicated?
And test file's directory path is already available ...in window.location.pathname :) But I understand you want to use it from HTML?

@Reinmar

This comment has been minimized.

Copy link
Contributor

Reinmar commented Jun 3, 2014

The problem I see is that if anything changes you're screwed. Just like now we are. Tests should be easy to write, easy to read and maintainable. Therefore constants controlled by Bender will be useful. They will make things simpler, cleaner and safer.

@Reinmar

This comment has been minimized.

Copy link
Contributor

Reinmar commented Jun 3, 2014

Note - by cleaner I was referring to very long /../../../../ paths which will need to be written without it.

@gregpabian gregpabian closed this in f202962 Jun 3, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.