Add code to parse the git branches out of the Trac RSS feed
Created attachment 270165 [details] Patch
Comment on attachment 270165 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270165&action=review > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:69 > + return (!commit.containsBranchLocation || commit.branchName === branchName || (commit.branchName instanceof Array && commit.branchName.includes(branchName))) && filter(commit); This does not seem like the correct approach. In particular, its weird that branchName could be either a String object or an Array object. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:172 > + var gitBranches = doc.evaluate("./branches", commitElement, null, XPathResult.STRING_TYPE).stringValue; > + if (gitBranches) { > + result.containsBranchLocation = true; > + result.branchName = gitBranches.split(", "); > + } We need a test case for a multi-branch commit. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:60 > + strictEqual(commits.length, 6, "should have 6 commits"); It's weird that there are six commits because on first glance this test only parses a single commit. I know that five of these commits are hardcoded in class MockTrac. Can we extract the hardcoded recorded commits from MockTrac into some kind of constant, maybe MockTrac.EXAMPLE_TRAC_COMMITS, such that MockTrac has no recorded commits by default? Unit tests that want to make use of this set of mock commits could then set MockTrac.recordedCommits = MockTrac.EXAMPLE_TRAC_COMMITS. And we would not make use of this set of commits for this test.
Created attachment 270409 [details] Adds git branches to the timeline RSS feed Alexey suggested checking this into the repository somewhere but I'm not sure where it belongs.
Created attachment 270426 [details] Patch
(In reply to comment #2) > Comment on attachment 270165 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=270165&action=review > > > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:69 > > + return (!commit.containsBranchLocation || commit.branchName === branchName || (commit.branchName instanceof Array && commit.branchName.includes(branchName))) && filter(commit); > > This does not seem like the correct approach. In particular, its weird that > branchName could be either a String object or an Array object. I addressed this in the latest patch. I refactored 'branchName' to be 'branches', changed the type of the variable to be an array, and updated the code that interacts with it accordingly. > > > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:172 > > + var gitBranches = doc.evaluate("./branches", commitElement, null, XPathResult.STRING_TYPE).stringValue; > > + if (gitBranches) { > > + result.containsBranchLocation = true; > > + result.branchName = gitBranches.split(", "); > > + } > > We need a test case for a multi-branch commit. I added a test case and corresponding assertions for a multi-branch commit. > > > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:60 > > + strictEqual(commits.length, 6, "should have 6 commits"); > > It's weird that there are six commits because on first glance this test only > parses a single commit. I know that five of these commits are hardcoded in > class MockTrac. Can we extract the hardcoded recorded commits from MockTrac > into some kind of constant, maybe MockTrac.EXAMPLE_TRAC_COMMITS, such that > MockTrac has no recorded commits by default? Unit tests that want to make > use of this set of mock commits could then set MockTrac.recordedCommits = > MockTrac.EXAMPLE_TRAC_COMMITS. And we would not make use of this set of > commits for this test. I did this in the latest patch as well.
Comment on attachment 270426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270426&action=review > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:169 > + var gitBranches = doc.evaluate("./branches", commitElement, null, XPathResult.STRING_TYPE).stringValue; I am assuming that doc.evaluate("./branches", commitElement, null, XPathResult.STRING_TYPE) always returns an object. We should add a test case for an entry that does not have <branches>. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:54 > + client.open('GET', 'test-fixture-git-trac-rss.xml', false); Nit: We use single quoted string literals here and double quoted string literals below. We should pick one quoting style for string literals and stick with it. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:58 > + client.onreadystatechange = function () { > + if (client.readyState === client.DONE) > + this.trac._loaded(client.responseXML); > + }.bind(this); Nit: We're underutilizing the the onreadystatechange listener. Since we only care if the load completed successfully, we could register a onload listener. See <https://xhr.spec.whatwg.org/#event-handlers> for a list of the event handlers that can be registered on XMLHttpRequest.
Created attachment 270448 [details] Patch
Comment on attachment 270448 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270448&action=review > Tools/ChangeLog:3 > + Add code to parse the git branches out of the Trac RSS feed. Nit: Remove the period at the end of this line so that it reflects the name of this bug. > Tools/ChangeLog:9 > + (Trac.prototype.commitsOnBranch): Update filter to check for git branches. Nit: git => Git > Tools/ChangeLog:10 > + (Trac.prototype._convertCommitInfoElementToObject): Parse git branches from the Trac RSS feed. Also changed "branchName" to Ditto. > Tools/ChangeLog:15 > + fixture that contains XML with a git branch. Ditto. > Tools/ChangeLog:16 > + * BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js: Added unit test to test parsing git Ditto. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/tests.js:59 > + strictEqual(commits.length, 3, "should have 2 commits"); 2 => 3
Landed: http://trac.webkit.org/changeset/195993