feat: log runs jsonl before db removal

[HuntTheSun]

Implementation of #282

Hi @timdegroot1996 , i hoped it's alright I moved forward with a draft implementation, I'm not sure how elegantly I handled arg passing and the custom-db tbh.

Questions

Is .jsonl an okay format?

I chose .jsonl because runs are removed one by one, so logging them one by one makes the most sense, which jsonl was invented to make easier. Batch collecting runs to log could cause inconsistencies if something is cancelled/exited during the execution. Also .jsonl is way easier to get the format right compared to .json when logging runs one by one.
Should I explore other formats or is jsonl alright?

---

Should I also implement feature in the server?

• Should I implement the API stuff for this feature also or should this only "live" in the one-shot cli?
• Should I implement a field in the UI to specify a "run_rm_log_path" ?

---

Thank you in advance!

[timdegroot1996]

I will look into .jsonl and your implementation tonight. I havent used it before so I can check if it makes sense then.

Regarding the other features let me review and then answer those as well.

Thanks for starting the implementation and you are of course free to do so! 😄

[timdegroot1996]

As always I reviewed the code but that all looks fine so I just added some ideas regarding the implementation. Let me know what you think of the suggestions!

Regarding your questions

Should I also implement feature in the server?
Should I implement the API stuff for this feature also or should this only "live" in the one-shot cli?
Should I implement a field in the UI to specify a "run_rm_log_path" ?

I think it can already be used in the server right? That part of the code should also pass through the same remove_run functions in the database and logremoved if the flag has been set.

So once you call the server like robotdashboard --server --logremoved all:path/to/abc.jsonl [other_options], shouldn't it automatically work? I don't think the admin UI needs components for this as long as my assumption is true that this works on startup for the server as well.

[HuntTheSun]

Hi @timdegroot1996 , I realized I need to ask some clarifying questions, sorry.

Ive pushed the changes to the inital draft in feat: rework --logremoved with type selection

1. (NOT IMPLEMENTED) Should I also add a "removed_at" timestamp to the jsonl?
   Currently I would append to the logfile if it exists, to prevent accidentally overwriting data, but if a user re-uses the same logfile he has no way of knowing when what was removed.

2. (IMPLEMENTED) For default logpath, is the current dir and output format like robot_removed_runs-TIMESTRING.json (TIMESTRING same as .html default) alright?

3. (IMPLEMENTED) Are you okay with AI-created tests? I let claude create some tests for this new feature, they look decent. Should I handwrite tests, let the AI do it or not at all?

By the way, the CI/CD robot-tests job "Initialize Browser library" kept on running for 20+ minutes.
I am not familiar with github CI/CD and their pricing or your local runner setup if you have one,
but you may want to look into a timeout for some jobs.
Have you also noticed that stage sometimes timing out? This is the first time I've had issues with it.

[HuntTheSun]

@timdegroot1996
Hi, I pushed fix/cicd: add rfbrowser init timeout to add a timeout to rfbrowser init, its not a "fix" per se though.
I hoped it would cancel the old pipeline, but it didn't. I hope it doesn't cost you anything until it runs into the hard default timeout (I think 6 hours by default).
Feel free to drop the commit (or have me rebase), I just wanted to make sure I don't open 42 different pipelines, each timing out too slowly and costing you.

Edit: I just realized my yml formatter changed the workflow quotation signs... sigh. Please tell me when I should fix it (if you wont drop the commit) , I will force push after you are okay with the current PR.

[timdegroot1996]

@HuntTheSun hahaha stuff like that happens, very relatable 😄!

As in regards to the costs, no worries. When you have a public github repo everything you do is free, even actions. So there won't be any costs for this.

Edit: I just realized my yml formatter changed the workflow quotation signs... sigh. Please tell me when I should fix it (if you wont drop the commit) , I will force push after you are okay with the current PR.

Does that mean that will fix the pipeline again? If so then you can just fix it on your end and I will simply squah and merge that commit. No need to do any complex stuff, the history isn't that important anyway.

By the way, are you done with this PR and can I start the review? It's status is still "Draft" so I want to make sure before I start.

[HuntTheSun]

@timdegroot1996
Thank you for easing my mind, I was a bit worried.
Sadly, the timeout doesn't fix it per se, just prevents very long runs if it hangs.
Im not sure why it hung though, have you ever run into this?
I think its ready for review, I have not implemented this yet though, if its alright you can review, if not I will add it.

(NOT IMPLEMENTED) Should I also add a "removed_at" timestamp to the jsonl?

So is it alright that there is no timestamp of deletion?

Thank you so much for your work!

[timdegroot1996]

Sadly, the timeout doesn't fix it per se, just prevents very long runs if it hangs.
Im not sure why it hung though, have you ever run into this?

Yeah no idea. Maybe the servers hosting the browser library binaries were having some issues. But the timeout is a great fix!

Ah I missed the comments below.

1. (NOT IMPLEMENTED) Should I also add a "removed_at" timestamp to the jsonl?
   Currently I would append to the logfile if it exists, to prevent accidentally overwriting data, but if a user re-uses the same logfile he has no way of knowing when what was removed.

2. (IMPLEMENTED) For default logpath, is the current dir and output format like robot_removed_runs-TIMESTRING.json (TIMESTRING same as .html default) alright?

3. (IMPLEMENTED) Are you okay with AI-created tests? I let claude create some tests for this new feature, they look decent. Should I handwrite tests, let the AI do it or not at all?

1. I think it's fine to not re-add it if it's already in the list, so having unique timestamps seems a bit overkill. We can always add it later if required.
2. I think adding the timestring might be a bit overkill, but I will let you know after reviewing the code some more. Leave it as is for now.
3. Yeah that's fine. I will review them as well and if they are okay we will keep them. If I find anything strange in the review I'll let you know as well.

I see you changed the status so I'll try to review it tonight! Maybe today if I have some spare time somewhere, I'll do my best.

[timdegroot1996]

Hi @HuntTheSun again sorry for the delay on the review. I found a few small improvements for which I opened some comments. Apart from that it looks good to me. I will merge this once you have finished the final 3 comments.

Regarding these comments:

1. (NOT IMPLEMENTED) Should I also add a "removed_at" timestamp to the jsonl?
   Currently I would append to the logfile if it exists, to prevent accidentally overwriting data, but if a user re-uses the same logfile he has no way of knowing when what was removed.
2. (IMPLEMENTED) For default logpath, is the current dir and output format like robot_removed_runs-TIMESTRING.json (TIMESTRING same as .html default) alright?
3. (IMPLEMENTED) Are you okay with AI-created tests? I let claude create some tests for this new feature, they look decent. Should I handwrite tests, let the AI do it or not at all?

1. Like I said, let's leave that out until someone actually requires it.
2. This is fine for now. I think keeping a single file without a timestamp would also be fine, since it's kind of strange to keep writing to different files. For me keeping this as a sort of single database json file would make more sense. But you can keep this as is unless you want to change it yourself as well.
3. Tests look good to me, all my unit tests were also AI generated so good that we upped the coverage 👍

[HuntTheSun]

Hi @timdegroot1996

Thank you for the review and don't worry about the delay :)

1. Left it out
2. I removed the timestamp, now appends to the same file without timestamp.
3. Nice, thank you

By the way, the "non-fix" timeout reared its ugly head, somehow the rf browser init job doesnt seem to like me :)
https://github.com/MarketSquare/robotframework-dashboard/actions/runs/26869921092/job/79242434817?pr=295

I don't really know how to approach the flaky job timeout...
Do you have any gut feeling on what might cause it?
Have you played around with using the browsers included in the container img so the download step may be skipped?

[HuntTheSun]

@timdegroot1996

Sorry I'm still struggling with the rfbrowser init workaround :/
I'm not sure if I will get it fixed soon, I think I will just repush and hope the error goes away, if thats okay with you?

Edit: Sorry if it bothers you, I just wanted to check why it wouldnt work so I made a bunch of commits changing the workflow, please drop them (or tell me to rebase once the rfbrowser stage is handled).

[HuntTheSun]

Hi,

I've given up for today with making the rfbrowser init play nice, I will just retry running it on friday, maybe somethings wrong with the VM or cache, unless you have a better idea or suggestion?
Using the browsers included in the container worked, but the reference pics were different,
so if this is gonna be an issue more often that may be a fix (I will open an issue if it turns out this is not a transient error)

[HuntTheSun]

Alright, it's friday and it's still timing out...
I really don't know what changed...
Do you have an idea what could cause this or how to fix it?
I marked this PR as a draft for now, I will undraft and rebase once the test situation is figured out if thats okay.
If you feel like the PR is okay the way it is feel free to undraft and squash/pick if you want.

thank you for your work and support :)

[timdegroot1996]

@HuntTheSun I'm sorry for responding so late. I've started a new assignment so my last week was way too busy.

Regarding the fixes, everything looks nice! So we can start looking toward merging this. Thanks for your extra work.

Regarding the pipeline, I have no idea why this is happening. I know there are a few things happening:

• I think playwright moved their CDN url for where the browsers are coming from and I'm not entirely sure if that has broken the pipeline. In that sense we could try upping the browser version in requirements-dev.txt to something more recent than robotframework-browser==19.12.4 I think version 20.0.0 just came out last night. Might be worth a shot.
• There is this option as well: https://github.com/MarketSquare/robotframework-browser, https://pypi.org/project/robotframework-browser-batteries/, which provides the same browsers but through a PYPI package, which could be a good alternative approach.
• Apart from that I have no clear Idea why this is failing nonstop. I'll try rerunning it today and investigating as well.

If I can't get it to fix somewhere today or tomorrow I might just merge with a failing pipeline as I think it's already fine as is. But let's see if we can get this to work.

[timdegroot1996]

@HuntTheSun welp that seemed to solve it 😅. Merging this PR once you set it to ready to merge, thanks again for the work!

[HuntTheSun]

Hi @timdegroot1996,

thank you for fixing the ci/cd, I wouldn't have figured that out in a long time.
I hope your assignment went well, no worries about the wait on my part, I am thankful for everything you do,
whenever you have time :)

I removed the commits documenting my heroic struggle and shameful defeat with the pipeline.
Thank you again!

[timdegroot1996]

@HuntTheSun you're very welcome and this was indeed a tough one. I got a bit lucky that updating the versions solved it right away...

I'm going to remember the term "heroic struggle" haha 😄!