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

SJRK-268: generalized custom theme tests to work for any custom theme #39

Merged
merged 11 commits into from Sep 25, 2019
Merged

SJRK-268: generalized custom theme tests to work for any custom theme #39

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
ef00953
SJRK-268: generalized custom theme tests to work for any custom theme
BlueSlug Aug 27, 2019
53fc4fb
SJRK-268: removed redundant callbacks from theme loading functions
BlueSlug Aug 28, 2019
886574c
SJRK-268: refactored custom theme file loading tests, including JS file
BlueSlug Aug 29, 2019
750481d
SJRK-268: more custom theme loading tests refactoring: model -> members
BlueSlug Aug 30, 2019
c19ab4b
Merge remote-tracking branch 'upstream/stories-floe-dev' into SJRK-268
BlueSlug Sep 3, 2019
7c70738
SJRK-268: adjusted custom theme script loading tests to rely on promise
BlueSlug Sep 3, 2019
8ec0d88
Merge remote-tracking branch 'upstream/stories-floe-dev' into SJRK-268
BlueSlug Sep 24, 2019
bfe9e21
SJRK-268: removed Cities Edit page link since editing has been disabled.
BlueSlug Sep 24, 2019
c27b52e
SJRK-268: removed invalid tests of clientConfig, made test more robust
BlueSlug Sep 24, 2019
bf1ae0d
SJRK-268: addressing review feedback, removing cruft
BlueSlug Sep 25, 2019
8509372
SJRK-268: removed unused files and CSS rules
BlueSlug Sep 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 3 additions & 8 deletions src/ui/storyTellingServerUI.js
View file
Open in desktop
Expand Up @@ -112,14 +112,7 @@ sjrk.storyTelling.loadTheme = function () {

$.get("/clientConfig").then(function (data) {
Copy link
Member

Choose a reason for hiding this comment

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

It would probably be a bit more readable/descriptive if the argument is called clientConfig instead of data.

if (data.theme !== data.baseTheme) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this condition is needed, it seems that you do the same thing in sjrk.storyTelling.loadCustomThemeFiles.

sjrk.storyTelling.loadCustomThemeFiles(data).then(function () {
loadPromise.resolve(data);
}, function (jqXHR, textStatus, errorThrown) {
loadPromise.reject({
isError: true,
message: errorThrown
});
});
fluid.promise.follow(sjrk.storyTelling.loadCustomThemeFiles(data), loadPromise);
} else {
loadPromise.resolve(data);
}
Expand Down Expand Up @@ -152,7 +145,9 @@ sjrk.storyTelling.loadCustomThemeFiles = function (clientConfig) {
href: cssUrl
}).appendTo("head");


$.getScript(scriptUrl, function () {
window.customThemeScriptLoaded = true;
Copy link
Member

Choose a reason for hiding this comment

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

You should still add it to your test namespace as opposed to adding something new to the global namespace. While it's unlikely to have a collision in the test, it's still safer and more explicit. Also, I didn't mean to add it to the callback of $.getScript but to a real script that is passed in by your test that calls sjrk.storyTelling.loadCustomThemeFiles, so that you can verify that the script was indeed injected.

Copy link
Member Author

Choose a reason for hiding this comment

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

I figured you meant that I should add it to the actual script file, but I was hesitant to do so since I'd be adding things to a real theme file.

The documentation for getScript indicates that the success callback function fires when the script has been loaded though not necessarily executed, so I can see some merit in checking with the method you're describing, but something just seems a little off. If I put something in the JS file that I check for, then I'll need to put it in every custom theme's JS file or otherwise the tests will fail for any theme that is missing that piece. Anyone who adds a new theme but not the associated flag will run into this issue.

I think this would be a perfect thing to add if/when I get mock responses for this working, in which case we can return any old arbitrary JS we like, including a flag. And in the meantime, I can properly namespace this flag or remove it altogether. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I didn't realize you were loading a real theme file, I thought it was loading a test one for the purpose of the tests. I'm really just suggesting that you verify that the JS file is loaded as you'd expect, and that was just one suggestion for how to approach that.

loadPromise.resolve(clientConfig);
}).fail(function (jqXHR, textStatus, errorThrown) {
loadPromise.reject({
Expand Down
36 changes: 12 additions & 24 deletions tests/ui/js/storyTellingServerUITests.js
View file
Open in desktop
Expand Up @@ -102,7 +102,7 @@ https://raw.githubusercontent.com/fluid-project/sjrk-story-telling/master/LICENS
}]
},{
name: "Test themed page loading functions with server config values",
expect: 4,
expect: 6,
sequence: [{
task: "sjrk.storyTelling.storyTellingServerUiTester.loadClientConfigFromServer",
args: ["/clientConfig", "{that}", "clientConfig.theme"],
Expand All @@ -113,25 +113,24 @@ https://raw.githubusercontent.com/fluid-project/sjrk-story-telling/master/LICENS
resolve: "jqUnit.assertDeepEq",
resolveArgs: ["The themed page load resolved as expected", "{that}.model.clientConfig", "{arguments}.0"]
},{
funcName: "sjrk.storyTelling.storyTellingServerUiTester.assertCustomCssLoaded",
funcName: "sjrk.storyTelling.storyTellingServerUiTester.verifyCustomThemeFilesLoaded",
args: ["{that}.model.clientConfig.theme", 1]
},{
funcName: "sjrk.storyTelling.loadCustomThemeFiles",
args: ["{that}.model.clientConfig"]
},{
funcName: "sjrk.storyTelling.storyTellingServerUiTester.assertCustomCssLoaded",
args: ["{that}.model.clientConfig.theme", 2]
task: "sjrk.storyTelling.loadCustomThemeFiles",
args: ["{that}.model.clientConfig"],
resolve: "sjrk.storyTelling.storyTellingServerUiTester.verifyCustomThemeFilesLoaded",
resolveArgs: ["{that}.model.clientConfig.theme", 2]
}]
}]
}]
});

sjrk.storyTelling.storyTellingServerUiTester.loadClientConfigFromServer = function (url, component, clientConfigPath) {
sjrk.storyTelling.storyTellingServerUiTester.loadClientConfigFromServer = function (url, testerComponent, clientConfigPath) {
var configPromise = fluid.promise();

$.get(url).then(function (data) {
if (data.theme !== data.baseTheme) {
fluid.set(component.model, clientConfigPath, data.theme);
testerComponent.applier.change(clientConfigPath, data.theme);
configPromise.resolve(data);
Copy link
Member

Choose a reason for hiding this comment

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

It looks like you are passing in the sjrk.storyTelling.storyTellingServerUiTester component and assigning the theme property of its clientConfig member with the theme property from the returned data object. But in the test, you do a jqUnit.assertDeepEq with clientConfig member against the data object returned by the resolved promise. I'm not sure of the validity of this test, or perhaps what you are trying to test may not be clear. It would seem that if the theme is invalid or possibly even missing that the test would still pass. If these are tests against a live instance, which I believe they are, we are making an assumption that the configuration for baseTheme and authoringEnabled hasn't been changed.

I see in the tests that you follow up this one with a call to sjrk.storyTelling.loadTheme which also fetches the config from the server. So I'm assuming what you are trying to do is to pre-fetch the config data to use as the expected value to compare the returned data from sjrk.storyTelling.loadTheme against. If this is correct, here are some thoughts:

  • They are essentially using the same mechanism for fetching this data, $.get("/clientConfig"). Can you source the data by another means; for example, can you fetch and source the data directly out of sjrk.storyTelling.server.config.json5 instead?
  • If you are just pre-fetching why not assign the whole data object to the clientConfig member?
    • If you can do this, then you can skip the assertion and just make the resolver a call to fluid.set and assign the value there instead of in the sjrk.storyTelling.storyTellingServerUiTester.loadClientConfigFromServer function.

Considerations for future JIRAs, if they don't already exist:

  • sjrk.storyTelling.loadTheme shouldn't hardcode the URL it uses to fetch the clientConfig
  • It might be a good idea not to serve the tests because we'll need to be careful not to write any tests that might modify the server. Also we probably don't want to serve the tests on a production instance.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fetching the clientConfig data directly from the configuration file is not possible, as these are client-side tests that aim to ensure these server-interfacing functions are working as expected. Exposing the config file would not be any different, functionally speaking, from the /clientConfig endpoint already present in the server.

With that in mind, I agree that checking that a value I've loaded from the server against another value loaded from exactly the same part of the server is invalid; at best, it's redundant. I will remove this part and implement the bypass you've suggested.

Regarding serving the tests outside of the development environment, there are currently no tests that make any changes to the server and no tests that affect the database directly. Can you elaborate on why we wouldn't want to expose the tests on the production container? Is it to avoid effects on that environment? FWIW the same methods/endpoints that can be harnessed by the tests are also usable/callable by any user at this point.

Copy link
Member

Choose a reason for hiding this comment

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

What I was suggesting about the alternate path for fetching the config file is that you ensure that the file is being retrieved correctly. That is, you verify the correct file is being fetched via that endpoint. I agree that it would be better as a server specific test.

If I'm remembering correctly, another issue, which is kind of separate, is that you can't run the server tests when an instance of the server is running, because they use the same port. This prevents running all the tests at once. What would be nice is if you could run the server, integration, and browser tests independently, but also run them all together using our testem setup.

Regarding the unit tests in production. Partly it's something that you probably don't want to expose to a user. It could be a security risk if it is leaking access or information about secured access to the server. I was more worried about accidentally writing a test that modified the live server, especially with integration tests. It sounds like the tests currently aren't doing this, but it's something that we'll have to be aware of.

} else {
configPromise.reject({
Expand Down Expand Up @@ -159,22 +158,11 @@ https://raw.githubusercontent.com/fluid-project/sjrk-story-telling/master/LICENS
mockServer.restore();
};

sjrk.storyTelling.storyTellingServerUiTester.assertCustomCssLoaded = function (expectedTheme, expectedInstanceCount) {
var expectedFileName = expectedTheme + ".css";

var cssFilesLinked = fluid.transform(fluid.getMembers($("link"), "href"), function (fileUrl) {
return fileUrl.split("/css/")[1];
});

var actualInstanceCount = 0;

fluid.each(cssFilesLinked, function (fileName) {
if (fileName === expectedFileName) {
actualInstanceCount++;
}
});
sjrk.storyTelling.storyTellingServerUiTester.verifyCustomThemeFilesLoaded = function (expectedTheme, expectedCssInstanceCount) {
var actualInstanceCount = $("link[href$=\"" + expectedTheme + ".css\"]").length;
jqUnit.assertEquals("Custom theme CSS file is linked the expected number of instances", expectedCssInstanceCount, actualInstanceCount);

jqUnit.assertEquals("Linked CSS files include the expected custom theme file the expected number of instances", expectedInstanceCount, actualInstanceCount);
jqUnit.assertTrue("The custom theme JS file has been loaded", window.customThemeScriptLoaded);
};

fluid.defaults("sjrk.storyTelling.storyTellingServerUiTest", {
Expand Down