Replace tape with tap - #1171

Closed
NatalieWolfe wants to merge 1 commit into
nodejs:masterfrom
NatalieWolfe:switch-to-tap
Closed

Replace tape with tap#1171
NatalieWolfe wants to merge 1 commit into
nodejs:masterfrom
NatalieWolfe:switch-to-tap

Conversation

@NatalieWolfe

Copy link
Copy Markdown
Contributor

The tap module is a more fully featured TAP framework that executes its tests in separate child processes.

Resolves#1123

@bnoordhuis

Copy link
Copy Markdown
Member

Thanks, this is great! I wonder though, does tap.test() actually run tests in a separate process? I glean from node-tap's source that only tap.spawn() does that.

@NatalieWolfe

Copy link
Copy Markdown
ContributorAuthor

Correct, tap.test doesn't run each in their own process, but $ tap test/test-* will spawn a separate child process for each file that matched. Was it per-individual-test that needed process isolation?

@bnoordhuis

Copy link
Copy Markdown
Member

Yes.

@NatalieWolfe

Copy link
Copy Markdown
ContributorAuthor

Is it possible to move the tests which need isolation into individual files? Which tests need this? I can look into doing that step as part of this ticket.

@bnoordhuis

Copy link
Copy Markdown
Member

I'm going to say 'all of them' - it's not really true but isolation never hurts. It's my original plan of attack for #1123.

@NatalieWolfe

Copy link
Copy Markdown
ContributorAuthor

Is that necessary up front? That seems like the kind of thing that could be resolved on a case-by-case basis when colliding tests are discovered. Moving individual tests into separate files isn't that hard, but seems like overkill if you don't actually need them isolated.

@bnoordhuis

Copy link
Copy Markdown
Member

Such things tend to go unnoticed, though. I raised #1123 after I found two unrelated tests had been clobbering each other undetected for some time.

Another anecdote: a project I was involved in had a bug caused by lack of isolation that stopped half the tests from running but it went undiscovered for a year because the CI was green all the time.

Comment threadpackage.json
},
"scripts": {
"test": "tape test/test-*"
"test": "tap test/test-*"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMHO need to add -Rtap

@refack

Copy link
Copy Markdown
Contributor

@NatalieWolfe do you need help? I can help...

@maclover7

Copy link
Copy Markdown
Contributor

ping @NatalieWolfe

@rvagg

Copy link
Copy Markdown
Member

needs to be updated, either @NatalieWolfe needs to revisit or someone else should open a PR, I'd +1 on merging this if it was ready

@cclauss

Copy link
Copy Markdown
Contributor

@NatalieWolfe Is this already done elsewhere or is there more to do here?

@richardlau

Copy link
Copy Markdown
Member

This was done in #1795.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stop using tape for tests

7 participants

@NatalieWolfe@bnoordhuis@refack@maclover7@rvagg@cclauss@richardlau
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Replace tape with tap - #1171

Closed
NatalieWolfe wants to merge 1 commit into
nodejs:masterfrom
NatalieWolfe:switch-to-tap
Closed

Replace tape with tap#1171
NatalieWolfe wants to merge 1 commit into
nodejs:masterfrom
NatalieWolfe:switch-to-tap

Conversation

@NatalieWolfe

Copy link
Copy Markdown
Contributor

The tap module is a more fully featured TAP framework that executes its tests in separate child processes.

Resolves#1123

@bnoordhuis

Copy link
Copy Markdown
Member

Thanks, this is great! I wonder though, does tap.test() actually run tests in a separate process? I glean from node-tap's source that only tap.spawn() does that.

@NatalieWolfe

Copy link
Copy Markdown
ContributorAuthor

Correct, tap.test doesn't run each in their own process, but $ tap test/test-* will spawn a separate child process for each file that matched. Was it per-individual-test that needed process isolation?

@bnoordhuis

Copy link
Copy Markdown
Member

Yes.

@NatalieWolfe

Copy link
Copy Markdown
ContributorAuthor

Is it possible to move the tests which need isolation into individual files? Which tests need this? I can look into doing that step as part of this ticket.

@bnoordhuis

Copy link
Copy Markdown
Member

I'm going to say 'all of them' - it's not really true but isolation never hurts. It's my original plan of attack for #1123.

@NatalieWolfe

Copy link
Copy Markdown
ContributorAuthor

Is that necessary up front? That seems like the kind of thing that could be resolved on a case-by-case basis when colliding tests are discovered. Moving individual tests into separate files isn't that hard, but seems like overkill if you don't actually need them isolated.

@bnoordhuis

Copy link
Copy Markdown
Member

Such things tend to go unnoticed, though. I raised #1123 after I found two unrelated tests had been clobbering each other undetected for some time.

Another anecdote: a project I was involved in had a bug caused by lack of isolation that stopped half the tests from running but it went undiscovered for a year because the CI was green all the time.

Comment threadpackage.json
},
"scripts": {
"test": "tape test/test-*"
"test": "tap test/test-*"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMHO need to add -Rtap

@refack

Copy link
Copy Markdown
Contributor

@NatalieWolfe do you need help? I can help...

@maclover7

Copy link
Copy Markdown
Contributor

ping @NatalieWolfe

@rvagg

Copy link
Copy Markdown
Member

needs to be updated, either @NatalieWolfe needs to revisit or someone else should open a PR, I'd +1 on merging this if it was ready

@cclauss

Copy link
Copy Markdown
Contributor

@NatalieWolfe Is this already done elsewhere or is there more to do here?

@richardlau

Copy link
Copy Markdown
Member

This was done in #1795.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stop using tape for tests

7 participants

@NatalieWolfe@bnoordhuis@refack@maclover7@rvagg@cclauss@richardlau
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Replace tape with tap - #1171

Closed
NatalieWolfe wants to merge 1 commit into
nodejs:masterfrom
NatalieWolfe:switch-to-tap
Closed

Replace tape with tap#1171
NatalieWolfe wants to merge 1 commit into
nodejs:masterfrom
NatalieWolfe:switch-to-tap

Conversation

@NatalieWolfe

Copy link
Copy Markdown
Contributor

The tap module is a more fully featured TAP framework that executes its tests in separate child processes.

Resolves#1123

@bnoordhuis

Copy link
Copy Markdown
Member

Thanks, this is great! I wonder though, does tap.test() actually run tests in a separate process? I glean from node-tap's source that only tap.spawn() does that.

@NatalieWolfe

Copy link
Copy Markdown
ContributorAuthor

Correct, tap.test doesn't run each in their own process, but $ tap test/test-* will spawn a separate child process for each file that matched. Was it per-individual-test that needed process isolation?

@bnoordhuis

Copy link
Copy Markdown
Member

Yes.

@NatalieWolfe

Copy link
Copy Markdown
ContributorAuthor

Is it possible to move the tests which need isolation into individual files? Which tests need this? I can look into doing that step as part of this ticket.

@bnoordhuis

Copy link
Copy Markdown
Member

I'm going to say 'all of them' - it's not really true but isolation never hurts. It's my original plan of attack for #1123.

@NatalieWolfe

Copy link
Copy Markdown
ContributorAuthor

Is that necessary up front? That seems like the kind of thing that could be resolved on a case-by-case basis when colliding tests are discovered. Moving individual tests into separate files isn't that hard, but seems like overkill if you don't actually need them isolated.

@bnoordhuis

Copy link
Copy Markdown
Member

Such things tend to go unnoticed, though. I raised #1123 after I found two unrelated tests had been clobbering each other undetected for some time.

Another anecdote: a project I was involved in had a bug caused by lack of isolation that stopped half the tests from running but it went undiscovered for a year because the CI was green all the time.

Comment threadpackage.json
},
"scripts": {
"test": "tape test/test-*"
"test": "tap test/test-*"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMHO need to add -Rtap

@refack

Copy link
Copy Markdown
Contributor

@NatalieWolfe do you need help? I can help...

@maclover7

Copy link
Copy Markdown
Contributor

ping @NatalieWolfe

@rvagg

Copy link
Copy Markdown
Member

needs to be updated, either @NatalieWolfe needs to revisit or someone else should open a PR, I'd +1 on merging this if it was ready

@cclauss

Copy link
Copy Markdown
Contributor

@NatalieWolfe Is this already done elsewhere or is there more to do here?

@richardlau

Copy link
Copy Markdown
Member

This was done in #1795.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stop using tape for tests

7 participants

@NatalieWolfe@bnoordhuis@refack@maclover7@rvagg@cclauss@richardlau
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Replace tape with tap - #1171

Closed
NatalieWolfe wants to merge 1 commit into
nodejs:masterfrom
NatalieWolfe:switch-to-tap
Closed

Replace tape with tap#1171
NatalieWolfe wants to merge 1 commit into
nodejs:masterfrom
NatalieWolfe:switch-to-tap

Conversation

@NatalieWolfe

Copy link
Copy Markdown
Contributor

The tap module is a more fully featured TAP framework that executes its tests in separate child processes.

Resolves#1123

@bnoordhuis

Copy link
Copy Markdown
Member

Thanks, this is great! I wonder though, does tap.test() actually run tests in a separate process? I glean from node-tap's source that only tap.spawn() does that.

@NatalieWolfe

Copy link
Copy Markdown
ContributorAuthor

Correct, tap.test doesn't run each in their own process, but $ tap test/test-* will spawn a separate child process for each file that matched. Was it per-individual-test that needed process isolation?

@bnoordhuis

Copy link
Copy Markdown
Member

Yes.

@NatalieWolfe

Copy link
Copy Markdown
ContributorAuthor

Is it possible to move the tests which need isolation into individual files? Which tests need this? I can look into doing that step as part of this ticket.

@bnoordhuis

Copy link
Copy Markdown
Member

I'm going to say 'all of them' - it's not really true but isolation never hurts. It's my original plan of attack for #1123.

@NatalieWolfe

Copy link
Copy Markdown
ContributorAuthor

Is that necessary up front? That seems like the kind of thing that could be resolved on a case-by-case basis when colliding tests are discovered. Moving individual tests into separate files isn't that hard, but seems like overkill if you don't actually need them isolated.

@bnoordhuis

Copy link
Copy Markdown
Member

Such things tend to go unnoticed, though. I raised #1123 after I found two unrelated tests had been clobbering each other undetected for some time.

Another anecdote: a project I was involved in had a bug caused by lack of isolation that stopped half the tests from running but it went undiscovered for a year because the CI was green all the time.

Comment threadpackage.json
},
"scripts": {
"test": "tape test/test-*"
"test": "tap test/test-*"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMHO need to add -Rtap

@refack

Copy link
Copy Markdown
Contributor

@NatalieWolfe do you need help? I can help...

@maclover7

Copy link
Copy Markdown
Contributor

ping @NatalieWolfe

@rvagg

Copy link
Copy Markdown
Member

needs to be updated, either @NatalieWolfe needs to revisit or someone else should open a PR, I'd +1 on merging this if it was ready

@cclauss

Copy link
Copy Markdown
Contributor

@NatalieWolfe Is this already done elsewhere or is there more to do here?

@richardlau

Copy link
Copy Markdown
Member

This was done in #1795.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stop using tape for tests

7 participants

@NatalieWolfe@bnoordhuis@refack@maclover7@rvagg@cclauss@richardlau
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Replace tape with tap - #1171

Closed
NatalieWolfe wants to merge 1 commit into
nodejs:masterfrom
NatalieWolfe:switch-to-tap
Closed

Replace tape with tap#1171
NatalieWolfe wants to merge 1 commit into
nodejs:masterfrom
NatalieWolfe:switch-to-tap

Conversation

@NatalieWolfe

Copy link
Copy Markdown
Contributor

The tap module is a more fully featured TAP framework that executes its tests in separate child processes.

Resolves#1123

@bnoordhuis

Copy link
Copy Markdown
Member

Thanks, this is great! I wonder though, does tap.test() actually run tests in a separate process? I glean from node-tap's source that only tap.spawn() does that.

@NatalieWolfe

Copy link
Copy Markdown
ContributorAuthor

Correct, tap.test doesn't run each in their own process, but $ tap test/test-* will spawn a separate child process for each file that matched. Was it per-individual-test that needed process isolation?

@bnoordhuis

Copy link
Copy Markdown
Member

Yes.

@NatalieWolfe

Copy link
Copy Markdown
ContributorAuthor

Is it possible to move the tests which need isolation into individual files? Which tests need this? I can look into doing that step as part of this ticket.

@bnoordhuis

Copy link
Copy Markdown
Member

I'm going to say 'all of them' - it's not really true but isolation never hurts. It's my original plan of attack for #1123.

@NatalieWolfe

Copy link
Copy Markdown
ContributorAuthor

Is that necessary up front? That seems like the kind of thing that could be resolved on a case-by-case basis when colliding tests are discovered. Moving individual tests into separate files isn't that hard, but seems like overkill if you don't actually need them isolated.

@bnoordhuis

Copy link
Copy Markdown
Member

Such things tend to go unnoticed, though. I raised #1123 after I found two unrelated tests had been clobbering each other undetected for some time.

Another anecdote: a project I was involved in had a bug caused by lack of isolation that stopped half the tests from running but it went undiscovered for a year because the CI was green all the time.

Comment threadpackage.json
},
"scripts": {
"test": "tape test/test-*"
"test": "tap test/test-*"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMHO need to add -Rtap

@refack

Copy link
Copy Markdown
Contributor

@NatalieWolfe do you need help? I can help...

@maclover7

Copy link
Copy Markdown
Contributor

ping @NatalieWolfe

@rvagg

Copy link
Copy Markdown
Member

needs to be updated, either @NatalieWolfe needs to revisit or someone else should open a PR, I'd +1 on merging this if it was ready

@cclauss

Copy link
Copy Markdown
Contributor

@NatalieWolfe Is this already done elsewhere or is there more to do here?

@richardlau

Copy link
Copy Markdown
Member

This was done in #1795.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stop using tape for tests

7 participants

@NatalieWolfe@bnoordhuis@refack@maclover7@rvagg@cclauss@richardlau
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Replace tape with tap - #1171

Closed
NatalieWolfe wants to merge 1 commit into
nodejs:masterfrom
NatalieWolfe:switch-to-tap
Closed

Replace tape with tap#1171
NatalieWolfe wants to merge 1 commit into
nodejs:masterfrom
NatalieWolfe:switch-to-tap

Conversation

@NatalieWolfe

Copy link
Copy Markdown
Contributor

The tap module is a more fully featured TAP framework that executes its tests in separate child processes.

Resolves#1123

@bnoordhuis

Copy link
Copy Markdown
Member

Thanks, this is great! I wonder though, does tap.test() actually run tests in a separate process? I glean from node-tap's source that only tap.spawn() does that.

@NatalieWolfe

Copy link
Copy Markdown
ContributorAuthor

Correct, tap.test doesn't run each in their own process, but $ tap test/test-* will spawn a separate child process for each file that matched. Was it per-individual-test that needed process isolation?

@bnoordhuis

Copy link
Copy Markdown
Member

Yes.

@NatalieWolfe

Copy link
Copy Markdown
ContributorAuthor

Is it possible to move the tests which need isolation into individual files? Which tests need this? I can look into doing that step as part of this ticket.

@bnoordhuis

Copy link
Copy Markdown
Member

I'm going to say 'all of them' - it's not really true but isolation never hurts. It's my original plan of attack for #1123.

@NatalieWolfe

Copy link
Copy Markdown
ContributorAuthor

Is that necessary up front? That seems like the kind of thing that could be resolved on a case-by-case basis when colliding tests are discovered. Moving individual tests into separate files isn't that hard, but seems like overkill if you don't actually need them isolated.

@bnoordhuis

Copy link
Copy Markdown
Member

Such things tend to go unnoticed, though. I raised #1123 after I found two unrelated tests had been clobbering each other undetected for some time.

Another anecdote: a project I was involved in had a bug caused by lack of isolation that stopped half the tests from running but it went undiscovered for a year because the CI was green all the time.

Comment threadpackage.json
},
"scripts": {
"test": "tape test/test-*"
"test": "tap test/test-*"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMHO need to add -Rtap

@refack

Copy link
Copy Markdown
Contributor

@NatalieWolfe do you need help? I can help...

@maclover7

Copy link
Copy Markdown
Contributor

ping @NatalieWolfe

@rvagg

Copy link
Copy Markdown
Member

needs to be updated, either @NatalieWolfe needs to revisit or someone else should open a PR, I'd +1 on merging this if it was ready

@cclauss

Copy link
Copy Markdown
Contributor

@NatalieWolfe Is this already done elsewhere or is there more to do here?

@richardlau

Copy link
Copy Markdown
Member

This was done in #1795.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stop using tape for tests

7 participants

@NatalieWolfe@bnoordhuis@refack@maclover7@rvagg@cclauss@richardlau
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Replace tape with tap - #1171

Closed
NatalieWolfe wants to merge 1 commit into
nodejs:masterfrom
NatalieWolfe:switch-to-tap
Closed

Replace tape with tap#1171
NatalieWolfe wants to merge 1 commit into
nodejs:masterfrom
NatalieWolfe:switch-to-tap

Conversation

@NatalieWolfe

Copy link
Copy Markdown
Contributor

The tap module is a more fully featured TAP framework that executes its tests in separate child processes.

Resolves#1123

@bnoordhuis

Copy link
Copy Markdown
Member

Thanks, this is great! I wonder though, does tap.test() actually run tests in a separate process? I glean from node-tap's source that only tap.spawn() does that.

@NatalieWolfe

Copy link
Copy Markdown
ContributorAuthor

Correct, tap.test doesn't run each in their own process, but $ tap test/test-* will spawn a separate child process for each file that matched. Was it per-individual-test that needed process isolation?

@bnoordhuis

Copy link
Copy Markdown
Member

Yes.

@NatalieWolfe

Copy link
Copy Markdown
ContributorAuthor

Is it possible to move the tests which need isolation into individual files? Which tests need this? I can look into doing that step as part of this ticket.

@bnoordhuis

Copy link
Copy Markdown
Member

I'm going to say 'all of them' - it's not really true but isolation never hurts. It's my original plan of attack for #1123.

@NatalieWolfe

Copy link
Copy Markdown
ContributorAuthor

Is that necessary up front? That seems like the kind of thing that could be resolved on a case-by-case basis when colliding tests are discovered. Moving individual tests into separate files isn't that hard, but seems like overkill if you don't actually need them isolated.

@bnoordhuis

Copy link
Copy Markdown
Member

Such things tend to go unnoticed, though. I raised #1123 after I found two unrelated tests had been clobbering each other undetected for some time.

Another anecdote: a project I was involved in had a bug caused by lack of isolation that stopped half the tests from running but it went undiscovered for a year because the CI was green all the time.

Comment threadpackage.json
},
"scripts": {
"test": "tape test/test-*"
"test": "tap test/test-*"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMHO need to add -Rtap

@refack

Copy link
Copy Markdown
Contributor

@NatalieWolfe do you need help? I can help...

@maclover7

Copy link
Copy Markdown
Contributor

ping @NatalieWolfe

@rvagg

Copy link
Copy Markdown
Member

needs to be updated, either @NatalieWolfe needs to revisit or someone else should open a PR, I'd +1 on merging this if it was ready

@cclauss

Copy link
Copy Markdown
Contributor

@NatalieWolfe Is this already done elsewhere or is there more to do here?

@richardlau

Copy link
Copy Markdown
Member

This was done in #1795.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stop using tape for tests

7 participants

@NatalieWolfe@bnoordhuis@refack@maclover7@rvagg@cclauss@richardlau
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Replace tape with tap - #1171

Closed
NatalieWolfe wants to merge 1 commit into
nodejs:masterfrom
NatalieWolfe:switch-to-tap
Closed

Replace tape with tap#1171
NatalieWolfe wants to merge 1 commit into
nodejs:masterfrom
NatalieWolfe:switch-to-tap

Conversation

@NatalieWolfe

Copy link
Copy Markdown
Contributor

The tap module is a more fully featured TAP framework that executes its tests in separate child processes.

Resolves#1123

@bnoordhuis

Copy link
Copy Markdown
Member

Thanks, this is great! I wonder though, does tap.test() actually run tests in a separate process? I glean from node-tap's source that only tap.spawn() does that.

@NatalieWolfe

Copy link
Copy Markdown
ContributorAuthor

Correct, tap.test doesn't run each in their own process, but $ tap test/test-* will spawn a separate child process for each file that matched. Was it per-individual-test that needed process isolation?

@bnoordhuis

Copy link
Copy Markdown
Member

Yes.

@NatalieWolfe

Copy link
Copy Markdown
ContributorAuthor

Is it possible to move the tests which need isolation into individual files? Which tests need this? I can look into doing that step as part of this ticket.

@bnoordhuis

Copy link
Copy Markdown
Member

I'm going to say 'all of them' - it's not really true but isolation never hurts. It's my original plan of attack for #1123.

@NatalieWolfe

Copy link
Copy Markdown
ContributorAuthor

Is that necessary up front? That seems like the kind of thing that could be resolved on a case-by-case basis when colliding tests are discovered. Moving individual tests into separate files isn't that hard, but seems like overkill if you don't actually need them isolated.

@bnoordhuis

Copy link
Copy Markdown
Member

Such things tend to go unnoticed, though. I raised #1123 after I found two unrelated tests had been clobbering each other undetected for some time.

Another anecdote: a project I was involved in had a bug caused by lack of isolation that stopped half the tests from running but it went undiscovered for a year because the CI was green all the time.

Comment threadpackage.json
},
"scripts": {
"test": "tape test/test-*"
"test": "tap test/test-*"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMHO need to add -Rtap

@refack

Copy link
Copy Markdown
Contributor

@NatalieWolfe do you need help? I can help...

@maclover7

Copy link
Copy Markdown
Contributor

ping @NatalieWolfe

@rvagg

Copy link
Copy Markdown
Member

needs to be updated, either @NatalieWolfe needs to revisit or someone else should open a PR, I'd +1 on merging this if it was ready

@cclauss

Copy link
Copy Markdown
Contributor

@NatalieWolfe Is this already done elsewhere or is there more to do here?

@richardlau

Copy link
Copy Markdown
Member

This was done in #1795.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stop using tape for tests

7 participants

@NatalieWolfe@bnoordhuis@refack@maclover7@rvagg@cclauss@richardlau