-
Notifications
You must be signed in to change notification settings - Fork 29
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
[FLAG] Adds keepAppOpen flag #212
Conversation
Thanks for the PR. Will review. however: "Rooibos does not currently support CI." This is not true at all.. I have a section in the docs saying how to do this, and I've been doing it on 5 projects for years. Perhaps read the docs, and then none of this work is needed? or perhaps you mean "it does not support my particular ci setup". If it's the latter, let me know and we can proceed with this pr; but let's not make changes if there is no real need. docs are here.. I have more robust roku-deploy checking scripts for this, these days, which use telnet node plugins, which imo are even better; I've created #213 to track this. |
Do you have the corresponding socket connection for this? I'm not really up for integrating a bespoke socket connection for your CI, if the community dont' benefit from it. If you share the corresponding piece of code to read this, I'll be happy to review and merge; but it has to be something other's can readily use. thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR would solve the problems we are facing as well, since parsing through telnet output has not proved stable for us. Also, sendHomeKeypress
doesn't seem to work on Roku OS 12.0 and just ends up inside the while loop, which the flag allows us to circumvent.
We'd be able to do something like this (or any other solution that reliably communicates to an external actor) in our test bundle's Main.bs file. Rooibos itself wouldn't really need to support sockets directly, just a way to expose the results and hand control back to the container app, as this PR suggests.
sub main()
_waitOnSocket(port.toInt())
Rooibos_init()
_emitReport(_buildReport(GetGlobalAA().scene.rooibosTestResult))
end sub
' CI Socket Integration
sub _waitOnSocket()
port = CreateObject("roAppInfo").getValue("rooibos_port")
if port = "" then return
messagePort = CreateObject("roMessagePort")
m.socket = CreateObject("roStreamSocket")
m.socket.setMessagePort(messagePort)
addr = CreateObject("roSocketAddress")
addr.setPort(port.toInt())
m.socket.setAddress(addr)
m.socket.notifyReadable(true)
x = m.socket.listen(1)
if NOT m.socket.eOK()
? "[ROOIBOS-V5]: Could not create socket."
return
end if
? "[ROOIBOS-V5]: Waiting for CI socket connection on port:" port
while true
msg = wait(0, messagePort)
if type(msg) = "roSocketEvent"
if m.socket.isReadable()
newConnection = m.socket.accept()
if newConnection = invalid
? "[ROOIBOS-V5]: Socket connection failed"
else
? substitute("[ROOIBOS-V5]:{0} connected! Running tests...", str(port))
m.connection = newConnection
return
end if
else
if newConnection <> invalid AND NOT newConnection.eOK()
? "[ROOIBOS-V5]: Closing connection on port:" port
newConnection.close()
end if
end if
end if
end while
end sub
sub _emitReport(report as Object)
if m.connection <> invalid
m.connection.sendStr(formatJSON(report))
end if
end sub
function _buildReport(result as Object) as Object
report = {}
report["success"] = NOT result.stats.hasFailures
report["totalTestCount"] = result.stats.ranCount
report["failedTestCount"] = result.stats.failedCount
report["tests"] = []
for each testSuite in result.testSuites
for each group in testSuite.groups
for each test in group.tests
testResult = {}
testResult["isFail"] = test.result.isFail
testResult["name"] = test.name
testResult["message"] = test.result.message
testResult["filePath"] = substitute("file://{0}:{1}", testSuite.filePath.trim(), stri(test.lineNumber).trim())
report.tests.push(testResult)
end for
end for
end for
return report
end function
stats: m.stats | ||
testSuites: m.testSuites | ||
} | ||
m.nodeContext.global.testsScene.rooibosTestResult = rooibosResult |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you pick storing results in the scene? Should we use GetGlobalAA()
instead? Or would there be no difference?
m.nodeContext.global.testsScene.rooibosTestResult = rooibosResult | |
GetGlobalAA().rooibosTestResult = rooibosResult |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to support a ci socket connection; but it's against the spirit of the project to facilitate people using it for their own ci, without providing examples of how others can benefit. I'll not merge any such solution that facilitates peoples private ci's without some help for other users. I at least need some documentation with a sample snippet of code. I'd much rather someone provides a sample js. I think that's a reasonable ask.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't recall. it was years ago, and I was solving really really hard problems while learning roku and trying to write this test framework. could have been something back in the os then that was a problem, could have been I was overwhelmed, or just didn't know enough yet. Happy to have this change if you're sure it doesnt break anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already shared examples of code on the brs side of things. The gulp side for my specific project (which I've been cleared to share) looks something like this:
var net = require('net');
var fs = require('fs');
module.exports = function (gulp, plugins) {
return cb => {
const ROKU_DEV_TARGET = process.env.ROKU_DEV_TARGET;
const RETRY_DELAY = 500;
const socket = net.Socket();
let remainingConnectionAttempts = 5;
getResults()
.then(parseResults)
.then(saveResults)
.then(results => {
if (results.success) {
console.log('Tests passed! \n')
} else {
console.log(`\n ${results.failedTestCount} ${results.failedTestCount > 1 ? 'Tests' : 'Test'} failed! \n`);
}
cb();
})
.catch(err => {
console.log(`\n Error: ${err} \n`);
cb(err);
});
function getResults() {
return new Promise((fulfil, reject) => {
let dataStr = '';
socket.setEncoding('utf8');
socket.setKeepAlive(false);
socket.setTimeout(180000, () => {
socket.end();
});
connect();
socket.on('connect', () => {
console.log(`\n Connected to ${ROKU_DEV_TARGET} \n`);
});
socket.on('data', data => {
dataStr = dataStr + data;
});
socket.on('error', err => {
errorMessage = `Unable to get response from box ${ROKU_DEV_TARGET}: ${err}`;
remainingConnectionAttempts--;
if (remainingConnectionAttempts < 0) {
reject(errorMessage);
return;
}
console.log(errorMessage);
console.log(`Retrying connection in ${RETRY_DELAY}ms. Remaining attempts: ${remainingConnectionAttempts}`)
setTimeout(connect, RETRY_DELAY);
});
socket.on('end', () => {
console.log('Closing Socket!');
fulfil(dataStr);
console.log(`\n Disconnected from ${ROKU_DEV_TARGET} \n `);
});
});
}
function parseResults(resultStream) {
return new Promise((fulfil, reject) => {
rooibosResult = JSON.parse(resultStream);
let xml = `
<testsuites>
<testsuite name="Rooibos" tests="${rooibosResult.totalTestCount}" failures="${rooibosResult.failedTestCount}">\n`;
rooibosResult.tests.forEach((test, index, tests) => {
if (!test.isFail) {
xml += `
<testcase name="Passed Test ${index}" classname="${test.name}"/>`
} else {
xml += `
<testcase name="${test.name}" classname="${test.filePath}-FAIL">
<failure message="${test.message}"/>
</testcase>`
}
xml += '\n';
});
xml += `
</testsuite>
</testsuites>
`
fulfil({
xml: xml,
success: rooibosResult.success,
empty: rooibosResult.totalTestCount == 0,
failedTestCount: rooibosResult.failedTestCount
});
});
}
function saveResults(results) {
return new Promise((fulfil, reject) => {
if (results.empty == false) {
const testResultsLoc = process.env.TEST_RESULTS_LOC || './source/tests/results/';
if (!fs.existsSync(testResultsLoc)) {
fs.mkdirSync(testResultsLoc);
}
fs.writeFile(testResultsLoc + 'test-results.xml', results.xml, err => {
if (err) {
reject(err);
}
console.log('\n Results saved to ' + testResultsLoc + 'test-results.xml \n ');
});
}
fulfil(results);
});
}
function connect() {
socket.connect(global.args.rooibosPort, ROKU_DEV_TARGET);
}
};
};
It just connects to the brs socket, waits for data, and generates an XML file which is then consumed by our Jenkins code (which I cannot share). I don't think this is the ultimate example, though. Because, in reality, there's many other ways to approach this. Ideas off the top of my head: a simple POST request on the brs side to a node-based HTTP server, or even saving results to the internal dev
registry so it can be read by another brs app.
Ultimately speaking, we've been really looking forward to switch to Rooibos v5 here at Sky and NBCU, with at least 4 different projects currently using some form/fork of an earlier version specifically to support our various CI needs. Instead of forking v5, we'd like to contribute by providing mechanisms to make these integrations easier, whilst keeping it open enough for different purposes, implementations and setups. I don't think this PR is asking to support a socket connection, but rather expose the test results so others can adapt their own solutions on top. My project's solution just happens to be socket support because that's what we currently use, but any other approach would be perfectly valid, and much easier to implement with this work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also disregard my suggestion, it was merely a question out of curiosity, and it should work just fine as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luis-soares-sky ... thank you for sharing the socket code. I'm approving this pr, and I'll follow up to document how others can use it, when I get a free moment.
I'll cut a release tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luis-soares-sky/ @twig2let can we resolve the conflicts please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just one failing unit test, then we're all good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a fix. It's not ideal but the BSConfig
defined in the test isn't getting passed into the ProgramBuilder
- it was commented out 13 months ago.
Tried passing the swv
var into the ProgramBuilder
but was getting different failures then.
thanks everyone. great contribution. |
Problem
Rooibos does not support my particular CI setup.
Solution
Enable CI support by exposing the test results on the
testsScene
's fieldrooibosTestResult
and adding a conditionalreturn
after the Rooibos runner has completed; this conditionalreturn
allows execution to return to the Main loop where projects can parse and send their tests results to a CI.Example CI Implementation
bsconfig-test.json
Test Manifest
Main.brs