Skip to content
This repository has been archived by the owner on Dec 8, 2024. It is now read-only.

Allowing you to test templates from MU co-located tests #33

Closed

Conversation

mansona
Copy link
Member

@mansona mansona commented Apr 5, 2018

This is a result of a pairing session with @iezer trying to get the integration tests on the emberconf2018 MU app to work: https://github.com/201-created/emberconf-schedule-2018/blob/integration-tests/src/ui/routes/application/-components/footer-prompt/component-test.js#L13

We have come up with 2 scenarios that it needs to work with:

  • components inside -components folders in routes
  • components inside other components' folders

This PR is reasonably hacky 😂 it's very much supposed to be a "here this works, how should we make it play nice" kind of implementation.

@Turbo87
Copy link
Member

Turbo87 commented Apr 5, 2018

@mansona I don't see a description anywhere of what exactly this is fixing 🤔

index.js Outdated
if (filename.includes('-components')) {
options.meta.moduleName = parseModuleName(filename);
} else if (filenameParts[2] === 'components' && filenameParts.length > 5) {
options.meta.moduleName = parseModuleName(filename);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it would be better if parseModuleName does the filename checking and returns null if the filename is irrelevant. So this could be

let filename = state.file.opts.filename;
let moduleName = parseModuleName(filename);
if (moduleName) {
  options.meta.moduleName = moduleName;
}

@mansona
Copy link
Member Author

mansona commented Apr 5, 2018

@Turbo87 I think this is trying to fix the Ember Template Compiler section of the main Quest issue but I don't have enough context to answer your question apart from that

@iezer
Copy link

iezer commented Apr 5, 2018

@Turbo87 in module unification, you may want to have an integration test for a private component, for example: /src/ui/routes/application/-components/foo-bar/component-test.js. You can imagine that inside of this test would be a call await render(hbs`{{foo-bar}}`). Right now this test would fail because the resolver would only look for this component in /src/ui/components/foo-bar. With this PR, this addon can detect that a component is private based on its path and then set the moduleName appropriately to simulate the component being rendered within the parent's template.

@mansona
Copy link
Member Author

mansona commented Apr 5, 2018

@iezer I have taken your advice, much nicer looking 🎉

I'm still not 100% happy with the style of the parseModuleName function but if we end up thinking that this is actually a goer then I (or anyone who wants to) can clean it up a bit 👍

const parseModuleName = require('../lib/parse-module-name');

describe('parse-module-name helper', function() {
it('should return applicaiton template when using a route local -component', function() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo here, should read application
Also can you add a few more test for the null case?

it('should return null for public components', function() {
  expect(parseModuleName('emberconf/src/ui/components/foo-bar/component-test.js').toEqual(null);
}
it('should return null for public components in test folder', function() {
  expect(parseModuleName('emberconf/tests/integration/components/foo-bar/component-test.js').toEqual(null);
}

@iezer
Copy link

iezer commented Apr 10, 2018

This repo has another PR #11 to support passing an options hash to the precompiler. It's 2 years old but I think we may want to merge that PR as well as this one. There may be other use cases where we want to pass in the options hash.

The two PRs can work together. If there is no options hash passed in, then we can automatically set the moduleName as done in this PR. But if there is an options hash passed in with a moduleName, then the argument takes precedence.

@mansona mansona changed the title [WIP] Allowing you to test templates from MU co-located tests Allowing you to test templates from MU co-located tests May 16, 2018
@mansona
Copy link
Member Author

mansona commented May 16, 2018

This PR is now ready to be reviewed and merged thanks to @mixonic and @iezer doing a pairing session on it 🎉

'use strict';

module.exports = function (filename) {
let parts = filename.split('/');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about windows?


if (lastPrivateCollectionIndex !== -1) {
// TODO: We likely do not need new array allocations for the prefix and
// collection parts
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this still WIP? otherwise this comment should be removed

return `${prefixString}/${collectionParts.join('/')}/template.hbs`;
}
return `${prefixString}/template.hbs`;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

    if (collectionParts.length > 3) {
      collectionParts.splice(-2);
      return `${prefixString}/${collectionParts.join('/')}/template.hbs`;

    } else if (collectionParts.length === 3) {
      return `${prefixString}/template.hbs`;
    }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also it seems that the route with collectionParts.length < 3 is not currently tested

@mansona
Copy link
Member Author

mansona commented May 17, 2018

@mixonic @iezer I solved one of the comments that @Turbo87 pointed out, if you could give me some more context on that section I'm happy to try it out for myself but I get the feeling that you might have more insight of what might need to be done there

@rwjblue
Copy link
Member

rwjblue commented Jun 2, 2018

FYI - #37 will introduce a conflict here, but hopefully it isn't too bad. Also, I made sure to avoid setting moduleName at the moment so that we don't have conflicting meanings before this PR lands.

@NullVoxPopuli
Copy link
Contributor

@Turbo87 @mansona, is this blocked by anything? anything I can do to help move it along?

@mansona
Copy link
Member Author

mansona commented Aug 21, 2018

The main thing that held me back was that @iezer and @mixonic had a few tests that they thought should be covered in this PR but I think they were covered by their commit on 15th May 🤔

Other than that all we need to do is resolve the conflicts and it should be good to go 🎉

@NullVoxPopuli
Copy link
Contributor

Other than that all we need to do is resolve the conflicts and it should be good to go tada

how complicated is that? :)

@mansona
Copy link
Member Author

mansona commented Aug 21, 2018

Unfortunately, I don't know, I haven't had a chance to look at this recently and I don't know if I'll get a chance this week 😖

@NullVoxPopuli
Copy link
Contributor

anyway I can give a look?

@mansona
Copy link
Member Author

mansona commented Aug 22, 2018

If there is anyone following this PR it has been continued here: #42

From what I can see #42 is ready to be considered for merging and it has covered the test cases necessary to merge

@mixonic
Copy link
Member

mixonic commented Aug 22, 2018

Closing this in favor of #42

@mixonic mixonic closed this Aug 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants