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

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

Merged
merged 11 commits into from Sep 25, 2019

Conversation

@BlueSlug
Copy link
Member

commented Aug 27, 2019

Addresses a bug I discovered after #38 was merged: the storyTellingServerUI.js tests depended on the theme being set to learningReflections, while that value will of course vary depending on the particular deployment of the Storytelling Tool and may have tripped up CI in the process.

I had trouble setting up a unit test with mock server responses via Sinon.js, so I have implemented it instead as an integration test with actual calls to the webserver to retrieve the value of the currently set custom theme. These tests will fail if no custom theme is set, but then again so will running the server.

@BlueSlug

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2019

@michelled or maybe @jobara to review

@jobara

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

@BlueSlug, I'm not sure what you mean by

These tests will fail if no custom theme is set, but then again so will running the server.

authoringEnabled: true
}));
sjrk.storyTelling.storyTellingServerUiTester.verifyCustomThemeLoading = function (callback) {
var loadPromise = fluid.promise();

This comment has been minimized.

Copy link
@jobara

jobara Aug 28, 2019

Member

Why does sjrk.storyTelling.loadThemedPage take a callback and return a promise? Shouldn't the promise be enough?

This comment has been minimized.

Copy link
@BlueSlug

BlueSlug Aug 28, 2019

Author Member

A fair point, I thought I'd addressed this already in SJRK-237. I'll look into this again!

resolve: "jqUnit.assertDeepEq",
resolveArgs: ["The themed page load resolved as expected", loadThemedPageTestCases.learningReflections, "{arguments}.0"]
resolveArgs: ["Custom theme was loaded successfully", "{that}.model.clientConfig", "{arguments}.0"]

This comment has been minimized.

Copy link
@jobara

jobara Aug 28, 2019

Member

It does seem strange, and slightly confusing that some of these use a mock server and others hit a real server. What was the issue you had with using the mock server for other tests? If you do need to have some tests against a mock and others against a real server, I think you should break these into at least different test sequences or completely different test environments.

mockServer = sinon.createFakeServer();
mockServer.respondImmediately = true;

mockServer.respondWith(url, [200, { "Content-Type": "application/json" }, responseData]);
mockServer.respondWith(url, [200, { "Content-Type": responseType }, JSON.stringify(testCase.clientConfig)]);

This comment has been minimized.

Copy link
@jobara

jobara Aug 28, 2019

Member

If you are only using clientConfig from testCase, you can just pass that in instead of the whole testCase.

@BlueSlug

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2019

@BlueSlug, I'm not sure what you mean by

These tests will fail if no custom theme is set, but then again so will running the server.

The tests that I've added in this PR will fail if the theme key isn't set in the server config file. I was actually mistaken about the other half; I thought that the server would crash on startup if theme was not set, but it will start and the site runs just fine.

$.get("/clientConfig").then(function (data) {
if (data.theme !== data.baseTheme) {
sjrk.storyTelling.loadCustomThemeFiles(callbackFunction, data).then(function () {
sjrk.storyTelling.loadCustomThemeFiles(data).then(function () {
loadPromise.resolve(data);
}, function (jqXHR, textStatus, errorThrown) {

This comment has been minimized.

Copy link
@jobara

jobara Aug 29, 2019

Member

fluid promises only return a single argument on reject. You can also see from the implementation from sjrk.storyTelling.loadCustomThemeFiles which only returns the following on an error.

{
            isError: true,
            message: errorThrown
}

What you can do here is just use fluid.promise.follow.


$.get(url).then(function (data) {
if (data.theme !== data.baseTheme) {
fluid.set(component.model, clientConfigPath, data.theme);

This comment has been minimized.

Copy link
@jobara

jobara Aug 29, 2019

Member

You shouldn't be setting model paths directly. If you need to update the model, the changes should always go through the applier. Perhaps in this case what you really want is a member.

This comment has been minimized.

Copy link
@BlueSlug

BlueSlug Aug 29, 2019

Author Member

@jobara I think a member is exactly what I want, but I found documentation for that to be a bit sparse. Can I just fluid.set it and refer to it at {that}.members...? Would it be wiser to continue using a model here and calling model.applier.change(...)?

This comment has been minimized.

Copy link
@jobara

jobara Aug 30, 2019

Member

@BlueSlug you've probably seen the brief members docs. Members are a way to add properties to a component in the defaults. These are hoisted to be direct properties of the component, similar to how an invoker is accessed. For example:

fluid.defaults("sample.component", {
    gradeNames: ["fluid.component"],
    members: {
         myProp: "some value"
    },
    invokers: {
        output: {
            funcName: "fluid.identity",
            args: ["{that}.myProp"]
        }
    }
});

var that = sample.component();
that.myProp; // returns "some value"
that. output(); // returns "some value"

that.myProp = "changed prop";
that.myProp; // returns "changed prop"
that.output(); // returns "changed prop"

Usually mutable aspects of the component are referring to their state and should be kept in the model, but for other cases you can use a member. It appears in your case it's more for record keeping. You could even keep a record of all of these server responses in an array and verify based on call number. In that way you can verify success and failure responses too. Although I guess you could do that with different model paths too.

You may also wish to consider moving the storing of the response to the test task. Then having another step in the sequence that verified the response. To that end, I guess you could just verify the response directly and not store it anywhere. I don't think it is used again, but I might have missed it.

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

var cssFilesLinked = fluid.transform(fluid.getMembers($("link"), "href"), function (fileUrl) {

This comment has been minimized.

Copy link
@jobara

jobara Aug 29, 2019

Member

I'm not 100% sure what you are getting from fluid.getMembers here, but I wonder if you could just do $("link").attr("href"). Taking that one step further because you only really want to get the links that match the injected CSS file, you could do $("link[href$=\"" + expectedFileName + "\"]"). And you would just use the length of this in your assertion. (see: https://api.jquery.com/attribute-ends-with-selector/ ).

I'm also not sure you really want to be able to inject the same file into the same document more than once. It might be nice for sjrk.storyTelling.loadCustomThemeFiles to just resolve if it is already there.

It also doesn't seem that you are testing the JS injections. You could do that by having a simple JS file that added a flag or something that could be retrieved from the public namespace, e.g "sjrk.storyTelling.storyTellingServerUiTester.customJSLoaded" or something like that.

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

This comment has been minimized.

Copy link
@jobara

jobara Aug 30, 2019

Member

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.

This comment has been minimized.

Copy link
@BlueSlug

BlueSlug Aug 30, 2019

Author Member

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?

This comment has been minimized.

Copy link
@jobara

jobara Sep 1, 2019

Member

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.

@@ -147,7 +147,7 @@ sjrk.storyTelling.loadCustomThemeFiles = function (clientConfig) {


$.getScript(scriptUrl, function () {
window.customThemeScriptLoaded = true;
sjrk.storyTelling.customThemeScriptLoaded = true;

This comment has been minimized.

Copy link
@jobara

jobara Sep 1, 2019

Member

As I mentioned in the other comment, the suggestion was purely for testing and not mean to change things in the actual files.

This comment has been minimized.

Copy link
@jobara

jobara Sep 1, 2019

Member

Although going along with your other comment about the callback firing when the script is loaded but not executed, what is actually coming from these JS files? Will you have any issue of trying to access something that hasn't yet been executed?

This comment has been minimized.

Copy link
@BlueSlug

BlueSlug Sep 3, 2019

Author Member

The usual contents of these custom JS files will be grades that are set up as extensions of the page grades, and they will be "instantiated" right after the script is loaded, so it's possible that there may be issues with trying to create a component before the script defining them has been executed. I haven't seen this happen to date.

This comment has been minimized.

Copy link
@BlueSlug

BlueSlug Sep 3, 2019

Author Member

Thinking about this further, the only reliable way I can think of testing (without causing a race condition) that the script was loaded and properly executed would be to mock the server response using Sinon and have the fetched script fire an event for which there is a listener test sequence item set up. Otherwise, whether the loadCustomThemeFiles promise resolves is all we can reliably determine and depend on, unless of course I'm missing something.

This comment has been minimized.

Copy link
@jobara

jobara Sep 3, 2019

Member

From what you've described, I think the race condition could happen in the real world. For example if the processing of the script is slow. I'm guessing that they are small enough that you may not be noticing this. That is, by the time the callback is complete the script has been executed.

@BlueSlug

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2019

I've filed SJRK-272 to address the potential race condition and have left a TODO note in the code as a reminder. I've also removed the flag to check whether the success callback was fired, as the promise resolution should be sufficient for this purpose.

$.get(url).then(function (data) {
if (data.theme !== data.baseTheme) {
testerComponent.clientConfig.theme = data.theme;
configPromise.resolve(data);

This comment has been minimized.

Copy link
@jobara

jobara Sep 4, 2019

Member

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.

This comment has been minimized.

Copy link
@BlueSlug

BlueSlug Sep 24, 2019

Author Member

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.

This comment has been minimized.

Copy link
@jobara

jobara Sep 25, 2019

Member

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.

@BlueSlug

This comment has been minimized.

Copy link
Member Author

commented Sep 24, 2019

Ready for more review

var loadPromise = fluid.promise();

var callbackFunction = typeof callback === "function" ? callback : fluid.getGlobalValue(callback);

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

This comment has been minimized.

Copy link
@jobara

jobara Sep 25, 2019

Member

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

var loadPromise = fluid.promise();

var callbackFunction = typeof callback === "function" ? callback : fluid.getGlobalValue(callback);

$.get("/clientConfig").then(function (data) {
if (data.theme !== data.baseTheme) {

This comment has been minimized.

Copy link
@jobara

jobara Sep 25, 2019

Member

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

baseTheme: "base",
authoringEnabled: true
}));
sjrk.storyTelling.storyTellingServerUiTester.loadClientConfigFromServer = function (url) {

This comment has been minimized.

Copy link
@jobara

jobara Sep 25, 2019

Member

I don't see this function being used anywhere. I think it's usage was removed in some of the refactoring and this can probably be dropped.

@@ -95,7 +95,7 @@ h3:empty {
align-items: center;
position: relative;
flex-wrap: wrap;
top: 1.75rem;
/* top: 1.75rem; */

This comment has been minimized.

Copy link
@jobara

jobara Sep 25, 2019

Member

can this be removed?

@@ -7,5 +7,5 @@
<li><a href="storyBrowse.html">{{localizedMessages.link_browse_text}}</a></li>
</ul>
</nav>
<a class="sjrk-st-edit-link" href="{{localizedMessages.editor_url}}">{{localizedMessages.link_edit_text}}</a>
{{!--<a class="sjrk-st-edit-link" href="{{localizedMessages.editor_url}}">{{localizedMessages.link_edit_text}}</a>--}}

This comment has been minimized.

Copy link
@jobara

jobara Sep 25, 2019

Member

Should remove the commented out code, or provide a comment for why it is needed.

@BlueSlug

This comment has been minimized.

Copy link
Member Author

commented Sep 25, 2019

Ready for more review.

I've also filed a number of Jiras and updated one, based on or discovered while working on your feedback:

  • SJRK-122 was updated to run server tests independently via testem
  • SJRK-298 for hardcoded URLs
  • SJRK-299 for cruft removal
  • SJRK-300 for hiding browser tests from public
  • SJRK-301 for Orator not reading alt text
@jobara jobara merged commit 8509372 into fluid-project:stories-floe-dev Sep 25, 2019
1 check passed
1 check passed
license/cla Contributor License Agreement is signed.
Details
@jobara

This comment has been minimized.

Copy link
Member

commented Sep 25, 2019

Merged at fc2c778

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