Smallish cleanups to docs & unit tests; improve a few tests #3429

peterflynn · 2013-04-12T21:40:05Z

SpecRunnerUtils:
- Improve docs
- Add assertion in case a mock Document is created without an Editor but then used in a way that would auto-create an Editor (which won't get cleaned up)
- In DocumentManager, add assertion in _destroyEditorIfUnneeded() since its name makes it easy to call with the wrong type (including via SRU.destroyMockEditor())
- Use waitsForDone() more
LanguageManager:
- Minor docs cleanups (incorrect usage of "i.e.", missing
  docs for new fileNames option, etc.)
- Remove a duplicated LanguageManager test (due to f67cfcd)
- Move filename->language mapping tests from Editor-test to LanguageManager-test
InstallExtensionDialog: Add unit tests for install dialog cases where installer comes back with a result after clicking Cancel but before Cancel times out, or comes back vary late after timeout once user has opted to close the dialog
StringMatch: Add a few more tests to ensure tricky cases don't break in the opposite direction as before (including the test suggested by Kevin in Fix exception when _noMatchCache contains the 'hasOwnProperty' property #3417)
DocumentManager: use CollectionUtils.forEach() in notifyPathNameChanged() (removing early-exit optimization that's probably unneeded; can reintroduce when CollectionUtils.some() lands)
FileIndexManager: add another Allow multiple calls to syncFileIndex to batch up #330 testcase as suggested by Glenn in Fix bug #330 (Calling FileIndexManager.getFileInfoList() fails if another such call in progress) #3064
HTMLCodeHints: rename test suite & remove unused var
CSSUtils: remove unused vars
Dialogs: tiny docs improvement

- Improve docs in SpecRunnerUtils - Add assertion in SRU in case a mock Document is created without an Editor but then used in a way that would auto-create an Editor (which won't get cleaned up) - Add assertion in _destroyEditorIfUnneeded() since its name makes it easy to call with the wrong type (including via SRU.destroyMockEditor()) - Rename HTML code hints suite & remove unused var - Minor docs cleanups in LanguageManager (incorrect usage of "i.e.", missing docs for new fileNames option, etc.) - Add unit tests for install dialog cases where installer comes back with a result after clicking Cancel but before Cancel times out, or comes back vary late after timeout once user has opted to close the dialog - Remove a duplicated LanguageManager test (due to f67cfcd) - Add a few more StringMatch tests to ensure tricky cases don't break in the opposite direction as before

* DocumentManager: use CollectionUtils.forEach() in notifyPathNameChanged() (removing early-exit optimization that's probably unneeded; can reintroduce when CollectionUtils.some() lands) * Move filename->language mapping tests from Editor-test to LanguageManager-test * Add another #330 testcase as suggested by Glenn in #3064 * Add StringMatch testcases as suggested by Kevin in #3417 * SpecRunnerUtils: use waitsForDone() more * CSSUtils-test: remove unused vars * Dialogs: tiny docs improvement

peterflynn · 2013-04-12T21:42:22Z

src/document/DocumentManager.js

-                        // If the path name is a file, there can only be one matched entry in the open document
-                        // list, which we just updated. Break out of the for .. in loop. 
-                        break;
-                    }


I had to remove this early-exit optimization for now. I doubt it has any discernible impact, but if I'm wrong we can reintroduce it once pull #3117 lands.

Now that #3117 has landed, this could use CollectionUtils.some.

I think in retrospect I'd actually prefer to leave it using CollectonUtils.forEach(), since it keeps the code a little simpler -- and there doesn't seem to be any real need to optimize here.

If there isn't nothing to optimize, then we can leave it like this. If not switching to CollectionUtils.some is as easy as adding return !isFolder inside the if and return false outside.

njx · 2013-05-10T01:56:12Z

Open for committers to review.

@bchintx

…eanups * origin/master: (1140 commits) Documentation nits from code review Updated by ALF automation. Updated by ALF automation. Unit test for Save As bug related to untitled documents (see note at #4468 (comment)). Fixed as a side effect of PR #4514. - Add unit test that @bchintx wrote to cover #4484. - Remove redundant timeout args in unit tests (same val as default) - Fix JSLint whitespace errors I noticed in ProjectManager Using "workingSetSort" instead of "workingSetAdd" in certain cases of addToWorkingSet() was problematic, since FileViewController attaches specific meaning only to the latter. Cosmetic tweaks to modal header background color, tables, and typography. Attempt a cleaner fix for bug #4484 (Working set broken after Save As / save untitled outside project): * addToWorkingSet() ensures item is moved to requested index, even if it was already in the working set at some other index * Add a force flag, so we can guarantee a redraw after suppressing it during removal earlier * Call handleFileAddToWorkingSet() directly from _doSaveAs() to simplify plumbing (no advantage to going through FileViewController in this case) code review comments add registry URLs add unit tests for update button and registryUpdate event fix async close of dialog in afterEach update extension manager spinner and registry UI unit tests. update sanity check string. when prompting to save-as an untitled document, activate that document in the editor Updated by ALF automation. Disabling failing unit tests code review comments part 2 change layout for extension manager messages Updated by ALF automation. oops, of course can't use CollectionUtils... ... Conflicts: src/language/LanguageManager.js src/widgets/Dialogs.js test/spec/Editor-test.js test/spec/FileIndexManager-test.js test/spec/InstallExtensionDialog-test.js test/spec/SpecRunnerUtils.js

TomMalbran · 2013-07-23T02:36:46Z

src/document/DocumentManager.js

            }
-        }
+        });
+
        // Delete the old keys
        for (i = 0; i < keysToDelete.length; i++) {


This for and the next one should use Array.forEach

TomMalbran · 2013-07-23T02:52:57Z

I just reviewed this request, so I guess I assigned it to myself.

It all looks great, just some minor comments.

Small docs & formatting tweaks. Fix merge cruft in Editor-test & CSSUtils-test.

peterflynn · 2013-07-23T04:54:57Z

test/spec/Editor-test.js

            });
+


The tests above were just testing LanguageManager functionality. I simplified & moved those into LanguageManager-test, and in their place added a single test here to make sure the Document's language actually carries through to the Editor's mode (which the old tests never checked).

peterflynn · 2013-07-23T04:56:57Z

@RaymondLim All merged up with master now, whenever you want to review. (Or @TomMalbran since it looks like you started looking at this, would you want to do a full review instead?)

Not important for the current sprint though, so later this week is fine either way.

TomMalbran · 2013-07-23T05:15:09Z

@peterflynn I did a full review and tested before your last commit. All new tests where passing and everything seems to have been working fine. I was even about to assign me as this was an Open request when I saw that @RaymondLim was assigned to it.

The new changes seem good, but instead of re-indenting, what about creating a new variable instead of replacing the old ones? With a new var statement, the strings can be aligned, and in some cases it makes it easier to read the tested code text.

…eanups * origin/master: (30 commits) turn off optimization in acorn (but not acorn_loose right now) cleanup unit test prefs temporarily switch to my tern Revert "Revert "Workaround for the Tern crash."" Update README.md Work around #4554 (Extension Manager font is hard to read on Windows), which is a Chromium bug, by avoiding the lightest font-weight on Win. Lighten the text slightly so it's still a little muted, like the design looks on Mac and with older CEFs on Win. Revert "Workaround for the Tern crash." Updated by ALF automation. Re-add toolbar hover. Fix some button appearance issues. Updated by ALF automation. * Fix bug #4548 - remove Save As from folder tree context menu * Fix exception thrown when File > Save As invoked with nothing open * Update docs for working set events to reflect PR #4450 integration tests for registerInlineEditProvider Fixes after review JSDoc fixes. Fix for extensions compare show error message and add safety check For #4535, show error message but not 'remove' link for bad extension in dev folder Refactor provider callback for export Generalize registerInlineEditProvider and registerInlineDocProvider to take an optional priority parameter change upper limit to 16000 ...

peterflynn · 2013-07-30T03:41:16Z

@TomMalbran I think this addresses all the other comments. I've reassigned to you. We haven't tagged Sprint 28, so no merging yet -- but hopefully tomorrow morning we'll be good to go. Feel free to hit merge whenever you want after that. Let me know if you need any other changes here.

TomMalbran · 2013-07-30T04:24:56Z

@peterflynn Everything looks fine. I found a small Nit, but on the previous code, so might fix it later. Will rerun the tests later and merge when we are in Sprint 28.

peterflynn · 2013-07-30T17:12:11Z

@TomMalbran Cool, ok -- we're open for merges now.

TomMalbran · 2013-07-30T20:11:06Z

src/language/LanguageManager.js

@@ -175,7 +175,7 @@ define(function (require, exports, module) {
    /**
     * Resolves a language ID to a Language object.
     * File names have a higher priority than file extensions. 
-     * @param {!string} id Identifier for this language, use only letters a-z or digits 0-9 and _ inbetween (i.e. "cpp", "foo_bar", "c99")
+     * @param {!string} id Identifier for this language: lowercase letters, digits, and _ separators (e.g. "cpp", "foo_bar", "c99")


Strings are not-nullable by default, so ! is not needed.
Same goes for lines all the other places where it is used as !string of it.

I'd prefer to leave this as-is for now -- we're very inconsistent about flagging nullability at all in most docs, so having it marked explicitly (even if matching the default) seems useful since it indicates someone definitely thought about whether it's nullable or not. If we ever do a big sweep and clean up all our JSDocs, it seems safe to start leaving it unspecified when matching the default... but maybe not just yet.

One of these days I still want to write an extension that runs Closure on your code to give type safety warnings -- sort of a JSLint/Hint on steroids. I think that would make it much easier for us to actually use Closure annotations properly... As it stands, I bet it's hard to find a file in our codebase that doesn't contain at least one incorrect type annotation :-(

We are really inconsistent about flagging nullability, but I started to fix them on some files I updated and I know others did too. Not a big thing, we can fix this another time.

It would be nice to have a Closure warning plugin.

TomMalbran · 2013-07-30T20:15:44Z

@peterflynn I found some more nits on the JSDoc types. Is all small ones, but since we are fixing the JSDocs here, we should fix these too. I can fix these and merge too. Everything else is fine, and tests are passing.

peterflynn · 2013-07-30T23:34:25Z

@TomMalbran Pushed the other cleanups

TomMalbran · 2013-07-30T23:49:46Z

Thanks for the cleanups. Merging now.

Smallish cleanups to docs & unit tests; improve a few tests

TomMalbran · 2013-07-31T04:56:22Z

test/spec/CSSUtils-test.js

+                    ".foo { color:red } \n" +
+                    "a::after { content: \" {\" attr(href) \"}\"; } \n" +
+                    "li::before { content: \"} h4 { color:black }\"; } \n" +
+                    "div { color:green }";


I just realized that these new JSLint errors are because we are now using an older version of JSLint that before and checked that these aren't errors on a newer version. So we probably shouldn't fix anymore of these errors and I can revert these changes in #4557.

Aha, that explains why no one noticed the errors in this file earlier. Good to know!

Noticed it when I saw a similar error in EditorCommandHandlers, which I know that didn't had JSLint errors before.

peterflynn added 2 commits April 12, 2013 13:38

peterflynn reviewed Apr 12, 2013
View reviewed changes

ghost assigned peterflynn and RaymondLim Jul 16, 2013

TomMalbran reviewed Jul 23, 2013
View reviewed changes

Fix JSLint/JSHint errors (some of which were on master).

7250b96

Small docs & formatting tweaks. Fix merge cruft in Editor-test & CSSUtils-test.

peterflynn reviewed Jul 23, 2013
View reviewed changes

ghost assigned TomMalbran Jul 30, 2013

Code review fixes - tweak docs, cleanup vars, convert for to forEach()

98f23f2

TomMalbran reviewed Jul 30, 2013
View reviewed changes

Code review: some more small JSDoc fixes

9df7830

TomMalbran added a commit that referenced this pull request Jul 30, 2013

Merge pull request #3429 from adobe/pflynn/unit-test-cleanups

a02f49b

Smallish cleanups to docs & unit tests; improve a few tests

TomMalbran merged commit a02f49b into master Jul 30, 2013

TomMalbran deleted the pflynn/unit-test-cleanups branch July 30, 2013 23:50

TomMalbran reviewed Jul 31, 2013
View reviewed changes

jasonsanjose mentioned this pull request Jul 31, 2013

fix failures in webplatform docs tests #4616

Merged

peterflynn mentioned this pull request Aug 2, 2013

Upgrade JSLint to the commit we were using before switching to a submodule #4642

Merged

core-ai-bot mentioned this pull request Aug 29, 2021

[CLOSED] Smallish cleanups to docs & unit tests; improve a few tests brackets-archive/bracketsIssues#3219

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Smallish cleanups to docs & unit tests; improve a few tests #3429

Smallish cleanups to docs & unit tests; improve a few tests #3429

peterflynn commented Apr 12, 2013

peterflynn Apr 12, 2013

TomMalbran Jul 23, 2013

peterflynn Jul 30, 2013

TomMalbran Jul 30, 2013

njx commented May 10, 2013

TomMalbran Jul 23, 2013

TomMalbran commented Jul 23, 2013

peterflynn Jul 23, 2013

peterflynn commented Jul 23, 2013

TomMalbran commented Jul 23, 2013

peterflynn commented Jul 30, 2013

TomMalbran commented Jul 30, 2013

peterflynn commented Jul 30, 2013

TomMalbran Jul 30, 2013

peterflynn Jul 30, 2013

TomMalbran Jul 30, 2013

TomMalbran commented Jul 30, 2013

peterflynn commented Jul 30, 2013

TomMalbran commented Jul 30, 2013

TomMalbran Jul 31, 2013

peterflynn Aug 1, 2013

TomMalbran Aug 1, 2013

Smallish cleanups to docs & unit tests; improve a few tests #3429

Smallish cleanups to docs & unit tests; improve a few tests #3429

Conversation

peterflynn commented Apr 12, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njx commented May 10, 2013

Choose a reason for hiding this comment

TomMalbran commented Jul 23, 2013

Choose a reason for hiding this comment

peterflynn commented Jul 23, 2013

TomMalbran commented Jul 23, 2013

peterflynn commented Jul 30, 2013

TomMalbran commented Jul 30, 2013

peterflynn commented Jul 30, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomMalbran commented Jul 30, 2013

peterflynn commented Jul 30, 2013

TomMalbran commented Jul 30, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment