-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
added keyword matching functionality for real-browser-monitor #6097
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
base: master
Are you sure you want to change the base?
Conversation
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 have not run the code but it seems solid.
A few corners need a bit more scrubbing.
Have you looked Into if we can reuse the text monitor condition logic (see the dns monitor) to reduce duplicate code?
I would prefer to have less code for maintenance reasons ^^
let textContent = await page.textContent("body"); | ||
|
||
if (textContent) { | ||
textContent = textContent.replace(/\s+/g, " ").trim(); |
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 this?
Could you add a comment?
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.
comment is added. it removes duplicate whitespaces to make the keyword matching more predictable when matching multiple words.
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.
how does it make more predicable?
What does this make more predictable?
Please actually document your reasoning why you think we should replace any whitespace at least one char wide with a space.
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.
html collapes duplicate spaces <div>Hello my <p> world</p></div>
would just read "hello my world" when rendered in html. maybe textContent would already handle that, but making sure its "hello my world" and not "Hello my world" makes it more predictable if the user is copying some phrase to match out of the browser instead of using the inspector to get the actual raw spacing.
maybe comment should be "remove duplicate whitespace as html also collapses them". Would that make more sense?
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.
Please test that this is necessary and if it is, document why this is necessary and why it is the correct way of doing things.
I don't think your logic is quite as solid as you think it is:
hello<pre> </pre>world
renders as:
hello
world
Also, how does this affect 	
and the other whitespace characters?
Please at least document why this is more expected than not doing this and how this is correct.
The PR now includes a backend test for the "real browser" monitor that runs an actual chromium browser. To be able to test the backend code without requiring an actual http:// or https:// website, i have allowed data:// urls. Data URLs are self-contained and are not subject to the local file:// inclusion vulnerability. Also while reviwing the code and debugging problems with the test never finishing, i noticed that the browser's page or context may never be explicitely removed if errors occur during its execution. I made it more defensive by using a finally clause and explicitely closing page and context in any case, which may or may not resolve #3788 Thank you for providing a review and feedback. I hope it is all resolved and good now. |
const availableKeywords = { | ||
found: "Hello", // Present in withHello pages | ||
notFound: "Missing", // Not present in any pages | ||
partial: "World", // Present in withHello pages |
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.
What does a partial
keyword match even mean?
How is this different from found
?
found: "Hello", // Present in withHello pages | ||
notFound: "Missing", // Not present in any pages | ||
partial: "World", // Present in withHello pages | ||
numeric: "123", // Present in withNumeric page |
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.
how is this different from found
?
notFound: "Missing", // Not present in any pages | ||
partial: "World", // Present in withHello pages | ||
numeric: "123", // Present in withNumeric page | ||
special: "àáâãäå" // Present in withSpecialChars page |
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.
How is this an interesting case to test, what are we testing here?
// Handle data: URLs which don't return a proper Response object | ||
const isDataUrl = url.protocol === "data:"; |
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 think this is usefull for monitoring purposes and for testing this is not very realistic.
Just spin up a local test server for the testcase, that should be fairly simple and not create such a weird branch
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.
The data: url is a feature supported by all browsers and commonly used in testing. The problem was just that the monitor was over-filtering data urls and not getting a http status code from them, what the monitor expects.
Its much simpler, faster and more reliable than a local test server.
- it saves the hassle of finding a free port for local binding and the error handling involved
- race conditions when the server is not ready to respond when the test needs it or these wired problems where the port is not freed immediately and cannot be bound again for some seconds.
- no problem with two tests running in parallel on the same network interface
- uses less resources
- causes less latency, runs faster
- is less code
while its true that data: urls are not the production use case, the difference in the code path regarding the monitor code is very small, so the coverage is basicly the same.
}; | ||
|
||
test("Real Browser Monitor Integration Tests", { | ||
skip: process.env.CI && !process.env.UPTIME_KUMA_ENABLE_BROWSER_TESTS |
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 this?
special: "àáâãäå" // Present in withSpecialChars page | ||
}; | ||
|
||
test("Real Browser Monitor Integration Tests", { |
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.
these testcases are currently really really hard to read.
Please DAMP them up a bit.
https://testing.googleblog.com/2019/12/testing-on-toilet-tests-too-dry-make.html
📋 Overview
What problem does this pull request address?
What features or functionality does this pull request introduce or enhance?
textContent()
method and performs keyword matching. This enables monitoring of dynamic web applications that rely heavily on JavaScript for content rendering.Resolves: Real Browser Allow keyword lookup #4068
🛠️ Type of change
📄 Checklist
🦿 LLM Assistance Declaration
This implementation was developed with assistance from an AI assistant (Claude) for:
I (the submitter) reviewed the code closely and tested it manually. Not so close attention was given to the provided tests. I also verified that the keyword input field appears on the other monitors ui.
🧪 Testing
test coverage added:
test/backend-test/test-real-browser-keyword.js
- Tests keyword logic, invert functionality, edge casestest/e2e/specs/monitor-form.spec.js
- Tests UI form behavior and monitor creationTest results:
🔧 Implementation Details
Files modified:
server/monitor-types/real-browser-monitor-type.js
- Core keyword checking logicsrc/pages/EditMonitor.vue
- UI form updatestest/backend-test/test-real-browser-keyword.js
- Unit tests (new)test/e2e/specs/monitor-form.spec.js
- E2E tests (enhanced)Key features:
page.textContent('body')
📷 Screenshots or Visual Changes
UI Modifications: Added keyword and invert keyword fields to real-browser monitor form, matching the existing pattern used in HTTP keyword monitors.
Changes made:
End result

https://vuetifyjs.com/en/getting-started/browser-support/ was choosen as the first request to this URL only references javascript and contains NO content.
This PR resolves a real world problem and extends the coverage of uptime kuma to web 2.0 applications with minimal changes.