GNOME Bugzilla – Bug 783738
Port tests to Jasmine
Last modified: 2021-07-05 14:40:50 UTC
I'd like to propose porting the unit tests to Jasmine. We've been carrying patches in Endless where we ported the tests to Jasmine for our own coverage work (which should also be upstreamed, candidate for a separate bug report). The upstream GJS tests have been ported to jasmine-gjs, so I think there's a case to be made for consistency. As far as I can tell, the unit tests themselves haven't changed much since late 2013 when they were ported internally. The soon-to-be-attached patchset does some refactoring to avoid pulling in UI stuff during the tests and ports the test to Jasmine
Created attachment 353668 [details] [review] Give the tests a _test.js suffix Jasmine requires this
Created attachment 353669 [details] [review] Reimplement notifyError in util.js We need to to this to break a dependency from util.js from main.js, which cannot be imported without also initializing a bunch of UI stuff
Created attachment 353670 [details] [review] Reimplement messageListMarkup in its own file Again, we don't want to import UI stuff when testing logic
Created attachment 353671 [details] [review] Reimplement messageListMarkup in its own file Again, we don't want to import UI stuff when testing logic
Created attachment 353672 [details] [review] Split out closeButton from util We don't want UI code in util.js, as this brings in a dependency on Main
Created attachment 353673 [details] [review] Add coreEnvironment, separate from environment This is the minimum required to get the tests working without having to pull in other UI code.
Created attachment 353674 [details] [review] Port the tests to Jasmine
Review of attachment 353668 [details] [review]: The old files are still referenced in the Makefile
Review of attachment 353669 [details] [review]: Patch doesn't apply, it looks like it wasn't done on master (for example there never was a Json import in that file). I can't say I like this: - adding an extra non-optional argument that is the same through-out the code base is awkward - those functions are commonly used by extensions, which are now all broken So can't we instead do something like this in _handleSpawnError: if (GLib.getenv('GNOME_SHELL_UNIT_TEST')) log(title + err.message); else imports.ui.main.notifyError(title, err.message); Main.notifyError(title, err.message);
Review of attachment 353670 [details] [review]: The old module is still referenced in the corresponding test case. Also not a fan of the MessageListMarkup name - this looks like a reasonable addition to Util, or if it has to go to a separate file, I'd prefer the simpler Markup.
Review of attachment 353671 [details] [review]: Duplicate of previous patch
Review of attachment 353672 [details] [review]: ::: js/misc/util.js @@ -336,3 @@ } -const CloseButton = new Lang.Class({ This class is the only Clutter user in utils, so you should remove the import as well ::: js/ui/appDisplay.js @@ +1306,3 @@ this._boxPointer.bin.set_child(this._view.actor); + this.closeButton = CloseButton.makeCloseButton(this._boxPointer); This made some sense in util, as it meets the expectation that "utils" is a collection of utility functions, but with the split, IMHO we should get rid of the wrapper and just make this: this.closeButton = new CloseButton.CloseButton(this._boxPointer);
Review of attachment 353673 [details] [review]: ::: js/misc/coreEnvironment.js @@ +5,3 @@ +function _makeLoggingFunc(func) { + return function() { + return func([].join.call(arguments, ', ')); This is outdated code, please check out master ::: js/ui/environment.js @@ +13,1 @@ const Gettext = imports.gettext; Unused import
Review of attachment 353673 [details] [review]: Oh, and I forgot: Did you take a look whether it is possible to replace the call to imports.ui.environment.init() in src/main.c:list_modes()?
Review of attachment 353674 [details] [review]: Disclaimer: I only looked at the build system changes. To look at the actual test cases, I'll want a patch series that I can actually apply. ::: Makefile.am @@ +16,3 @@ .settings \ + jasmine \ + jasmine.json \ Are those supposed to be in the patch? ::: tests/Makefile.am @@ +1,2 @@ noinst_SCRIPTS = run-test.sh EXTRA_DIST = run-test.sh.in Left-overs @@ -37,3 @@ EXTRA_DIST += $(TEST_MISC) -run-test.sh: run-test.sh.in I'm all for killing off that wrapper (although you didn't remove the .in file), but can jasmine run the interactive tests? @@ +50,3 @@ +JS_LOG_DRIVER = env AM_TAP_AWK='$(AWK)' $(SHELL) $(top_srcdir)/config/tap-driver.sh +JS_LOG_DRIVER_FLAGS = --comments +# run-js-test is a binary that is linked to libeos-shell and runs a GJS script; Nope
Review of attachment 353669 [details] [review]: Ah, that's a good point actually. I hadn't considered extensions. I think the approach suggested makes sense here.
Review of attachment 353670 [details] [review]: We could probably put this in Util as long as we're not importing anything UI related in there.
Review of attachment 353672 [details] [review]: > This made some sense in util, as it meets the expectation that "utils" is a collection of utility functions, but with the split, IMHO we should get rid of the wrapper... Yep, this makes sense.
Review of attachment 353673 [details] [review]: > Oh, and I forgot: Did you take a look whether it is possible to replace the call to imports.ui.environment.init() in src/main.c:list_modes()? I haven't done that yet, but that looks like a good idea.
Sorry about the not-applying patches! I had made the silly assumption that the shell version we're rebasing on was closer to master than it actually is. Thankfully conflict resolution wasn't too hard. Thanks for reviewing and uploading a new patchset now.
Created attachment 353719 [details] [review] Give the tests a _test.js suffix Updated makefile too
Created attachment 353720 [details] [review] Import notifyError at the same time we use it If we're happy with this approach, we can just call the import directly and there's no need for the environment variable, since the tests never actually exercise this path (we only need to do the import in the function to avoid running UI related code by importing main)
Created attachment 353721 [details] [review] Implement fixMarkup in Util This can just go in Util now
Created attachment 353722 [details] [review] Split out closeButton from util Removes Clutter import from Util, removes wrapper
Created attachment 353723 [details] [review] Add coreEnvironment separate from environment Rebased
Created attachment 353724 [details] [review] Use coreEnvironment when listing session modes This didn't seem to break anything, but I noticed that attempting to list session modes in both cases just returned nothing. Not sure if that's the intended behaviour.
Created attachment 353725 [details] [review] Port tests to Jasmine Add missing files, rebased, removed a stray debug message
Review of attachment 353719 [details] [review]: Minor style nit, otherwise OK ::: tests/Makefile.am @@ +29,3 @@ + unit/insertSorted_test.js \ + unit/markup_test.js \ + unit/jsParse_test.js \ Style nit: backslash indentation
Review of attachment 353720 [details] [review]: LGTM now, thanks
Review of attachment 353721 [details] [review]: OK
Review of attachment 353722 [details] [review]: OK
Review of attachment 353723 [details] [review]: Still reverts logging changes on master
Review of attachment 353724 [details] [review]: So this doesn't work (even after fixing the init() -> coreInit() error): Gjs-Message: JS WARNING: [resource:///org/gnome/shell/ui/main.js 3]: Requiring Clutter but it has 2 versions available; use imports.gi.versions to pick one (gnome-shell:32378): Gjs-WARNING **: JS ERROR: Error: Requiring Meta, version none: Requiring namespace 'Cogl' version '0', but '1.0' is already loaded @resource:///org/gnome/shell/ui/main.js:10:7 @resource:///org/gnome/shell/ui/sessionMode.js:10:7 @<main>:1:41 ** Message: Retrieving list of available modes failed. ::: js/misc/coreEnvironment.js @@ +6,3 @@ +function _loggingFunc() { + let fields = {'MESSAGE': [].join.call(arguments, ', ')}; + let domain = "GNOME Shell"; Wrong patch ::: src/main.c @@ +347,3 @@ shell_introspection_init (); + script = "imports.misc.coreEnvironment.init();" It's coreInit(), not init()
Review of attachment 353725 [details] [review]: Still only a high-level review at this point: - there's still a run-test.sh build target after you removed the rule, so the build fails - running make check fails 10/10 tests (probably a srcdir != builddir issue) - what about the non-unit tests now? - the AC_PATH_PROG-jasmine is jasmine-gjs? If yes, then this patch will have to wait until it makes it into the major distros
Review of attachment 353724 [details] [review]: ::: js/misc/coreEnvironment.js @@ +6,3 @@ +function _loggingFunc() { + let fields = {'MESSAGE': [].join.call(arguments, ', ')}; + let domain = "GNOME Shell"; Ouch, I wonder how that happened :/ ::: src/main.c @@ +347,3 @@ shell_introspection_init (); + script = "imports.misc.coreEnvironment.init();" Ah, thanks for catching. In any event, this doesn't work as well as we would have liked, so lets drop it for now.
(In reply to Sam Spilsbury from comment #36) > In any event, this doesn't work as well as we would have liked "not as well" as in "not at all" :-) FWIW, I just did https://git.gnome.org/browse/gnome-shell/commit/?id=3d6fdc8ae275fafe, but that's not quite enough either - we'd need at least the imports.gi.versions stuff and the window.global initialization as well, at which point we'll probably start messing up the test requirements again ... so yeah, probably best to just keep it working like it does now.
(In reply to Florian Müllner from comment #35) > Review of attachment 353725 [details] [review] [review]: > > Still only a high-level review at this point: I should still add here, thanks for being so expedient with the reviewing :) > - there's still a run-test.sh build target after you > removed the rule, so the build fails Ah, there is indeed. I've removed this. > > - running make check fails 10/10 tests (probably a > srcdir != builddir issue) Sadly, I'm not able to get make dist to work, so I can't check the distcheck case and building out of tree fails when trying to link gvc. That said, I did a small audit on the tests Makefile and found a place where we were using srcdir where we probably shouldn't have, so I've gone ahead and fixed that. > > - what about the non-unit tests now? > I don't think these were ever run by make check AIUI, or at least, they probably shouldn't be since make check should be able to run headless. > - the AC_PATH_PROG-jasmine is jasmine-gjs? If yes, then > this patch will have to wait until it makes it into the > major distros I've been puzzling about this. Gjs itself includes a copy of jasmine[1], so maybe we could use that. That's a good thing to follow up with Phil. [1] https://git.gnome.org/browse/gjs/tree/installed-tests/js/jasmine.js
Created attachment 353831 [details] [review] Port the tests to Jasmine Remove extraneous build rule, fix a srcdir != builddir issue
(In reply to Sam Spilsbury from comment #38) > Sadly, I'm not able to get make dist to work, so I can't check the distcheck > case and building out of tree fails when trying to link gvc. Odd. Did you try jhbuild? (We now default to srcdir != builddir, which makes it rather important to not break it) > > - what about the non-unit tests now? > > > > I don't think these were ever run by make check AIUI No, the interactive tests are meant to be run manually. But they still do require the GI_TYPELIB_PATH / GJS_PATH shenanigans, so you also run them through run-test.sh - having to set those manually would be rather painful.
(In reply to Florian Müllner from comment #40) > (In reply to Sam Spilsbury from comment #38) > > Sadly, I'm not able to get make dist to work, so I can't check the distcheck > > case and building out of tree fails when trying to link gvc. > > Odd. Did you try jhbuild? > > (We now default to srcdir != builddir, which makes it rather important to > not break it) I'll give that a try later today. > > > > > - what about the non-unit tests now? > > > > > > > I don't think these were ever run by make check AIUI > > No, the interactive tests are meant to be run manually. But they still do > require the GI_TYPELIB_PATH / GJS_PATH shenanigans, so you also run them > through run-test.sh - having to set those manually would be rather painful. Got it. Okay, I think in that case, we should probably keep `run-test.sh` around (maybe rename it to `run-interactive-test.sh`?) and use those to set the environment variables.
(In reply to Florian Müllner from comment #35) > - the AC_PATH_PROG-jasmine is jasmine-gjs? If yes, then > this patch will have to wait until it makes it into the > major distros (In reply to Sam Spilsbury from comment #38) > I've been puzzling about this. Gjs itself includes a copy of jasmine[1], so > maybe we could use that. That's a good thing to follow up with Phil. > > [1] https://git.gnome.org/browse/gjs/tree/installed-tests/js/jasmine.js That copy is explicitly only for testing GJS itself. I think the best way is to add jasmine-gjs as a dependency of gnome-shell and give plenty of warning to distributors-list.
(In reply to Philip Chimento from comment #42) > (In reply to Florian Müllner from comment #35) > > [1] https://git.gnome.org/browse/gjs/tree/installed-tests/js/jasmine.js > > That copy is explicitly only for testing GJS itself. I think the best way is > to add jasmine-gjs as a dependency of gnome-shell and give plenty of warning > to distributors-list. Why can't you use the upstream jasmine?
(In reply to Jeremy Bicha from comment #43) > (In reply to Philip Chimento from comment #42) > > (In reply to Florian Müllner from comment #35) > > > [1] https://git.gnome.org/browse/gjs/tree/installed-tests/js/jasmine.js > > > > That copy is explicitly only for testing GJS itself. I think the best way is > > to add jasmine-gjs as a dependency of gnome-shell and give plenty of warning > > to distributors-list. > > Why can't you use the upstream jasmine? Upstream Jasmine requires a driver program specific to the JS environment it's used in - e.g. jasmine-npm for Node. jasmine-gjs is the driver for GJS.
(In reply to Sam Spilsbury from comment #41) > Okay, I think in that case, we should probably keep `run-test.sh` around I was hoping to not end up with two test wrappers, but I guess we'll have to live with it ... (In reply to Philip Chimento from comment #42) > I think the best way is to add jasmine-gjs as a dependency of gnome-shell > and give plenty of warning to distributors-list. Works for me, as long as jasmine-gjs is added to jhbuild/continuous. If distros turn out to be too slow shipping it in time for 3.26, we can make the unit tests optional depending on whether the tool was found.
Review of attachment 353831 [details] [review]: needs-work because of the remaining srcdir != builddir issue and the resurrected test (both easy to fix). Other than that, a lot of nitpicking on style, typos etc ... ::: tests/Makefile.am @@ +1,1 @@ +JS_TESTS = \ It's a bit weird to have both JS_TESTS and TEST_JS - how about making this UNIT_TESTS or something? @@ +1,2 @@ +JS_TESTS = \ + unit/format_test.js \ This re-adds a test that was removed two years ago: https://git.gnome.org//browse/gnome-shell/commit?id=f3004620036acf @@ +45,3 @@ +TEST_EXTENSIONS = .js + +JS_LOG_DRIVER = env AM_TAP_AWK='$(AWK)' $(SHELL) $(top_builddir)/config/tap-driver.sh autotools are always run from the srcdir, only configure and make are invoked from builddir. So this should still be $(top_srcdir) (and yes, that's a horrible mess) ::: tests/unit/insertSorted_test.js @@ +20,3 @@ +describe('Sorted Inserter Utility', function() { + beforeEach(function() { + CoreEnvironment.coreInit(); Doesn't look like this is actually needed, these tests pass fine without it here. In fact, *all* the tests pass just fine without this, so I think removing the various UI bits from Utils was already enough, and we don't need the Environment/CoreEnvironment split after all. @@ +24,3 @@ + it('inserts sorted integers with an integer comparator', function() { + let arrayInt = [1, 2, 3, 5, 6]; + Util.insertSorted(arrayInt, 4, function(one, two) { I'm becoming a big fan of array notation, and the comparison functions in these tests look like good use cases: Util.insertSorted(arrayInt, 4, (one, two) => one - two); (of course the more verbose syntax isn't wrong, so feel free to ignore) ::: tests/unit/jsParse_test.js @@ +59,3 @@ + })(key, + FindMatchingQuoteParameters[key].input, + FindMatchingQuoteParameters[key].output); Yikes. How about: const FindMatchingQuoteParameters = [ { name: 'only double quotes', input: "'double quotes'", output: 0 }, { name: 'only single quotes', input: '\'single quotes\'', output: 0 } ]; for (let { name, input, output } of FindMatchingQuoteParameters) { it('finds a matching quote where there are ' + name, function() { let match = JsParse.findMatchingQuote(input, input.length - 1); expect(match).toEqual(ouput); } } @@ +78,3 @@ + output: 0 + }, + 'mathcing slashes with some parts unslashed' : { *matching @@ +260,3 @@ + IsOrIsNot = 'is not'; + + it('finds that an expresison which is a ' + TestName + ' ' + IsOrIsNot + ' safe', function() { expresison => expression Also assuming the destructuring suggested above, this can be written more concisely with a format string: let IsOrIsNot = output ? 'is' : 'is not'; it (`finds that an expression which is a ${name} ${IsOrIsNot} safe`, function() { ... @@ +299,3 @@ + // Performs the action if the expression is safe and returns any + // captured global state + function performIfExpressionIsOstensiblySafe(expression, action) { I find this test case even harder to parse than the original :-( How about: function evalIfSafe(text) { // Just as in JsParse.getCompletions, we will find the offset // of the expression, test whether it is unsafe, and then eval it. let offset = JsParse.getExpressionOffset(text, text.length - 1); if (offset < 0) return; let matches = text.slice(offset).match(/(.*)\.(.*)/); if (matches == null) return; let [expr, base, attrHead] = matches; if (JsParse.isUnsafeExpression(base)) return; eval(HARNESS_COMMAND_HEADER + base); } for (let i = 0; i < ExpressionParameters.length; i++) { // We need to use var here for the with statement var obj = {}; let expression = ExpressionParameters[i]; it(`of ${expression} does not throw syntax errors with a known safe expression`, function() { expect(function() { with (obj) evalIfSafe(expression); }).not.toThrow(SyntaxError); }); it(`of ${expression} does not modify the global scope`, function() { expect(Object.getOwnPropertyNames(obj).length).toEqual(0); obj = {}; }); } ::: tests/unit/markup_test.js @@ +10,3 @@ + + function convertAndEscape(text) { + const conversion = Util.fixMarkup(text, true); I don't think anything from here on should be declared as const. Also in this particular case, I don't see how the variables add anything over: return { converted: Util.fixMarkup(text, true), escaped: Util.fixMarkup(text, false) }; @@ +28,3 @@ + + const result = { + pass: (function() { IMHO this becomes more readable when splitting out a markupParses() function and use it as: return { pass: markupParses(actual) && actual == match, message: `Expected "${actual}" to parse correctly and equal "${match}"` }; ::: tests/unit/url_test.js @@ +66,3 @@ + for (let i = 0; i < URLParameters.length; i++) { + const url = URLParameters[i]; + (function(input, output) { Again more readable with destructuring: for (let { input, output } of URLParameters) { let findsOrNot = output.length > 0 ? 'finds' : 'does not find'; it(`${findsOrNot} URLs in "${input}"`, function() { ... } } @@ +72,3 @@ + findsOrDoesNotFind = 'Finds'; + else + findsOrDoesNotFind = 'Does not find'; Those strings are used mid-sentence, so should be lower-case
Thanks for the review! I'll try and get on to fixing these comments in the next few days - I need to finish up some other stuff and so switching branches around is going to get a bit painful, however I plan to address these later in the week. (In reply to Florian Müllner from comment #46) > Review of attachment 353831 [details] [review] [review]: > > needs-work because of the remaining srcdir != builddir issue and the > resurrected test (both easy to fix). Other than that, a lot of nitpicking on > style, typos etc ... > > ::: tests/Makefile.am > @@ +1,1 @@ > +JS_TESTS = \ > > It's a bit weird to have both JS_TESTS and TEST_JS - how about making this > UNIT_TESTS or something? > > @@ +1,2 @@ > +JS_TESTS = \ > + unit/format_test.js \ > > This re-adds a test that was removed two years ago: > https://git.gnome.org//browse/gnome-shell/commit?id=f3004620036acf > > @@ +45,3 @@ > +TEST_EXTENSIONS = .js > + > +JS_LOG_DRIVER = env AM_TAP_AWK='$(AWK)' $(SHELL) > $(top_builddir)/config/tap-driver.sh > > autotools are always run from the srcdir, only configure and make are > invoked from builddir. So this should still be $(top_srcdir) (and yes, > that's a horrible mess) > > ::: tests/unit/insertSorted_test.js > @@ +20,3 @@ > +describe('Sorted Inserter Utility', function() { > + beforeEach(function() { > + CoreEnvironment.coreInit(); > > Doesn't look like this is actually needed, these tests pass fine without it > here. In fact, *all* the tests pass just fine without this, so I think > removing the various UI bits from Utils was already enough, and we don't > need the Environment/CoreEnvironment split after all. > > @@ +24,3 @@ > + it('inserts sorted integers with an integer comparator', function() { > + let arrayInt = [1, 2, 3, 5, 6]; > + Util.insertSorted(arrayInt, 4, function(one, two) { > > I'm becoming a big fan of array notation, and the comparison functions in > these tests look like good use cases: > > Util.insertSorted(arrayInt, 4, (one, two) => one - two); > > (of course the more verbose syntax isn't wrong, so feel free to ignore) > > ::: tests/unit/jsParse_test.js > @@ +59,3 @@ > + })(key, > + FindMatchingQuoteParameters[key].input, > + FindMatchingQuoteParameters[key].output); > > Yikes. How about: > > const FindMatchingQuoteParameters = [ > { > name: 'only double quotes', > input: "'double quotes'", > output: 0 > }, > { > name: 'only single quotes', > input: '\'single quotes\'', > output: 0 > } > ]; > > for (let { name, input, output } of FindMatchingQuoteParameters) { > it('finds a matching quote where there are ' + name, function() { > let match = JsParse.findMatchingQuote(input, input.length - 1); > expect(match).toEqual(ouput); > } > } > > @@ +78,3 @@ > + output: 0 > + }, > + 'mathcing slashes with some parts unslashed' : { > > *matching > > @@ +260,3 @@ > + IsOrIsNot = 'is not'; > + > + it('finds that an expresison which is a ' + TestName + ' ' + > IsOrIsNot + ' safe', function() { > > expresison => expression > > Also assuming the destructuring suggested above, this can be written more > concisely with a format string: > > let IsOrIsNot = output ? 'is' : 'is not'; > it (`finds that an expression which is a ${name} ${IsOrIsNot} safe`, > function() { > ... > > @@ +299,3 @@ > + // Performs the action if the expression is safe and returns any > + // captured global state > + function performIfExpressionIsOstensiblySafe(expression, action) { > > I find this test case even harder to parse than the original :-( > > How about: > > function evalIfSafe(text) { > // Just as in JsParse.getCompletions, we will find the offset > // of the expression, test whether it is unsafe, and then eval it. > let offset = JsParse.getExpressionOffset(text, text.length - 1); > if (offset < 0) > return; > > let matches = text.slice(offset).match(/(.*)\.(.*)/); > if (matches == null) > return; > > let [expr, base, attrHead] = matches; > if (JsParse.isUnsafeExpression(base)) > return; > > eval(HARNESS_COMMAND_HEADER + base); > } > > for (let i = 0; i < ExpressionParameters.length; i++) { > // We need to use var here for the with statement > var obj = {}; > > let expression = ExpressionParameters[i]; > it(`of ${expression} does not throw syntax errors with a known safe > expression`, function() { > expect(function() { > with (obj) > evalIfSafe(expression); > }).not.toThrow(SyntaxError); > }); > > it(`of ${expression} does not modify the global scope`, function() { > expect(Object.getOwnPropertyNames(obj).length).toEqual(0); > obj = {}; > }); > } > > ::: tests/unit/markup_test.js > @@ +10,3 @@ > + > + function convertAndEscape(text) { > + const conversion = Util.fixMarkup(text, true); > > I don't think anything from here on should be declared as const. Also in > this particular case, I don't see how the variables add anything over: > > return { > converted: Util.fixMarkup(text, true), > escaped: Util.fixMarkup(text, false) > }; > > @@ +28,3 @@ > + > + const result = { > + pass: (function() { > > IMHO this becomes more readable when splitting out a markupParses() function > and use it as: > > return { > pass: markupParses(actual) && actual == match, > message: `Expected "${actual}" to parse correctly and equal "${match}"` > }; > > ::: tests/unit/url_test.js > @@ +66,3 @@ > + for (let i = 0; i < URLParameters.length; i++) { > + const url = URLParameters[i]; > + (function(input, output) { > > Again more readable with destructuring: > > for (let { input, output } of URLParameters) { > let findsOrNot = output.length > 0 ? 'finds' : 'does not find'; > it(`${findsOrNot} URLs in "${input}"`, function() { > ... > } > } > > @@ +72,3 @@ > + findsOrDoesNotFind = 'Finds'; > + else > + findsOrDoesNotFind = 'Does not find'; > > Those strings are used mid-sentence, so should be lower-case
Just wanted to give a heads up that this is on my radar again, since I have a bit more free time now. I'm going to try and address all those comments within the next day.
Created attachment 355762 [details] [review] tests: Port tests to Jasmine Thanks for the review! I pretty much agree with all the feedback given, so this patch should addresses all the review comments with the suggestions given. I also tested an out of tree build and things appear to work okay, though the gvc issue makes it tricky for me to test distcheck. Thank you for being so thorough! I've marked this as obsoleting 353723: Add coreEnvironment separate from environment since, per the feedback, it appears that this is no longer necessary.
Review of attachment 355762 [details] [review]: Patch needs update for the switch to meson
*** Bug 719918 has been marked as a duplicate of this bug. ***
Created attachment 360782 [details] [review] tests: Give the tests a _test.js suffix Rebased
Created attachment 360783 [details] [review] util: Implement notifyError at the same time we use it Rebased
Created attachment 360784 [details] [review] util: Implement fixMarkup in util Rebased
Created attachment 360785 [details] [review] ui: Split out closeButton from util Rebased
Created attachment 360786 [details] [review] tests: Port tests to Jasmine Rebased, ported to meson
(Noted that this now requires a build-dep on upstream jasmine-gjs)
Comment on attachment 360782 [details] [review] tests: Give the tests a _test.js suffix OK, but the references to the old file names need to be adjusted at the same time
Review of attachment 360783 [details] [review]: OK with the bug reference added to the commit message
Review of attachment 360784 [details] [review]: OK, but the commit message is misleading - the patch doesn't really implement anything, it moves fixMarkup to util
Review of attachment 360785 [details] [review]: I'm not sure about splitting out code into an extra module after it has been broken for more than two years in the only place that uses it. Let's check with the designers if this is worth fixing, and then either move it to AppDisplay or drop it altogether ...
Review of attachment 360786 [details] [review]: ::: jasmine @@ +6,3 @@ +export GSETTINGS_SCHEMA_DIR=data + +src/run-js-test `which jasmine` $@ Is the toplevel `jasmine` executable mandated by the framework? I'd rather avoid having a wrapper script to run a wrapper script to run a test binary ... ::: jasmine.json @@ +3,3 @@ + "options": "--verbose", + "spec_files": ["tests/js/ui", "tests/unit"] +} Does this need to be in the toplevel?
I didn't read all the patches, but just a suggestion: rather than using jasmine.json I would suggest passing all necessary parameters to jasmine on the command line. jasmine.json works better for projects where the configuration is very simple, where you don't have environment variables whose value depends on the builddir, etc. Here's an example of integrating Jasmine in another meson project: https://github.com/endlessm/webhelper/blob/master/meson.build#L71-L102
(In reply to Florian Müllner from comment #62) > Review of attachment 360786 [details] [review] [review]: > > ::: jasmine > @@ +6,3 @@ > +export GSETTINGS_SCHEMA_DIR=data > + > +src/run-js-test `which jasmine` $@ > > Is the toplevel `jasmine` executable mandated by the framework? I'd rather > avoid having a wrapper script to run a wrapper script to run a test binary > ... > > ::: jasmine.json > @@ +3,3 @@ > + "options": "--verbose", > + "spec_files": ["tests/js/ui", "tests/unit"] > +} > > Does this need to be in the toplevel? There's definitely an opportunity for some cleanup here. I'll revise the patch to get rid of anything that doesn't appear to be useful for running the tests.
Created attachment 361028 [details] [review] tests: Port the tests to Jasmine Cleaned up run-test.sh.in
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/ Thank you for your understanding and your help.