From ba472711be2a6a0e9adc49c22f1fafdb8d865166 Mon Sep 17 00:00:00 2001 From: Oliver Giles Date: Fri, 1 Nov 2019 07:27:34 +0200 Subject: [PATCH] refactor: remove run page json type hack this hack tried to avoid sending unnecessary data to the frontend, but it was more trouble than it's worth --- src/http.cpp | 10 ++-------- src/http.h | 2 +- src/laminar.cpp | 8 ++++---- src/monitorscope.h | 13 +++++++------ test/laminar-functional.cpp | 28 ++++++++++++++++++++++++++++ 5 files changed, 42 insertions(+), 19 deletions(-) diff --git a/src/http.cpp b/src/http.cpp index f575177..488d576 100644 --- a/src/http.cpp +++ b/src/http.cpp @@ -278,16 +278,10 @@ kj::Promise Http::startServer(kj::Timer& timer, kj::OwnlistenHttp(*listener).attach(cleanupPeers(timer)).attach(kj::mv(listener)).attach(kj::mv(server)); } -void Http::notifyEvent(const char *type, const char *data, std::string job, uint run) +void Http::notifyEvent(const char *data, std::string job) { for(EventPeer* c : eventPeers) { - if(c->scope.wantsStatus(job, run) - // The run page also should know that another job has started - // (so maybe it can show a previously hidden "next" button). - // Hence this small hack: - // TODO obviate - || (std::string(type)=="job_started" && c->scope.type == MonitorScope::Type::RUN && c->scope.job == job)) - { + if(c->scope.wantsStatus(job)) { c->pendingOutput.push_back("data: " + std::string(data) + "\n\n"); c->fulfiller->fulfill(); } diff --git a/src/http.h b/src/http.h index 909a244..1d37755 100644 --- a/src/http.h +++ b/src/http.h @@ -40,7 +40,7 @@ public: kj::Promise startServer(kj::Timer &timer, kj::Own &&listener); - void notifyEvent(const char* type, const char* data, std::string job = nullptr, uint run = 0); + void notifyEvent(const char* data, std::string job = nullptr); void notifyLog(std::string job, uint run, std::string log_chunk, bool eot); private: diff --git a/src/laminar.cpp b/src/laminar.cpp index 5170319..740d618 100644 --- a/src/laminar.cpp +++ b/src/laminar.cpp @@ -574,7 +574,7 @@ std::shared_ptr Laminar::queueJob(std::string name, ParamMap params) { .startObject("data") .set("name", name) .EndObject(); - http->notifyEvent("job_queued", j.str(), name.c_str()); + http->notifyEvent(j.str(), name.c_str()); assignNewJobs(); return run; @@ -666,7 +666,7 @@ bool Laminar::tryStartRun(std::shared_ptr run, int queueIndex) { } j.EndArray(); j.EndObject(); - http->notifyEvent("job_started", j.str(), run->name.c_str(), run->build); + http->notifyEvent(j.str(), run->name.c_str()); return true; } } @@ -755,7 +755,7 @@ void Laminar::runFinished(Run * r) { populateArtifacts(j, r->name, r->build); j.EndArray(); j.EndObject(); - http->notifyEvent("job_completed", j.str(), r->name, r->build); + http->notifyEvent(j.str(), r->name); http->notifyLog(r->name, r->build, "", true); // erase reference to run from activeJobs. Since runFinished is called in a // lambda whose context contains a shared_ptr, the run won't be deleted @@ -797,7 +797,7 @@ bool Laminar::handleBadgeRequest(std::string job, std::string &badge) { db->stmt("SELECT result FROM builds WHERE name = ? ORDER BY number DESC LIMIT 1") .bind(job) .fetch([&](int result){ - rs = (RunState) result; + rs = RunState(result); }); if(rs == RunState::UNKNOWN) return false; diff --git a/src/monitorscope.h b/src/monitorscope.h index 2b48850..987eafe 100644 --- a/src/monitorscope.h +++ b/src/monitorscope.h @@ -41,19 +41,20 @@ struct MonitorScope { order_desc(true) {} - // whether this scope wants status information about the given job or run + // whether this scope wants status information for the specified job bool wantsStatus(std::string ajob, uint anum = 0) const { if(type == HOME || type == ALL) return true; - if(type == JOB) return ajob == job; - if(type == RUN) return ajob == job && anum == num; - return false; + else return ajob == job; + // we could have checked that the run number matches, but actually the + // run page needs to know about a non-matching run number in order to + // know whether to display the "next" arrow. } Type type; std::string job; - uint num = 0; + uint num ; // sorting - uint page = 0; + uint page; std::string field; bool order_desc; }; diff --git a/test/laminar-functional.cpp b/test/laminar-functional.cpp index 6afac07..0956cce 100644 --- a/test/laminar-functional.cpp +++ b/test/laminar-functional.cpp @@ -73,3 +73,31 @@ TEST_F(LaminarFixture, JobNotifyHomePage) { EXPECT_STREQ("foo", job_completed["data"]["name"].GetString()); } +TEST_F(LaminarFixture, OnlyRelevantNotifications) { + defineJob("job1", "true"); + defineJob("job2", "true"); + + auto esHome = eventSource("/"); + auto esJobs = eventSource("/jobs"); + auto es1Job = eventSource("/jobs/job1"); + auto es2Job = eventSource("/jobs/job2"); + auto es1Run = eventSource("/jobs/job1/1"); + auto es2Run = eventSource("/jobs/job2/1"); + + auto req1 = client().runRequest(); + req1.setJobName("job1"); + ASSERT_EQ(LaminarCi::JobResult::SUCCESS, req1.send().wait(ioContext.waitScope).getResult()); + auto req2 = client().runRequest(); + req2.setJobName("job2"); + ASSERT_EQ(LaminarCi::JobResult::SUCCESS, req2.send().wait(ioContext.waitScope).getResult()); + ioContext.waitScope.poll(); + + EXPECT_EQ(7, esHome->messages().size()); + EXPECT_EQ(7, esJobs->messages().size()); + + EXPECT_EQ(4, es1Job->messages().size()); + EXPECT_EQ(4, es2Job->messages().size()); + + EXPECT_EQ(4, es1Run->messages().size()); + EXPECT_EQ(4, es2Run->messages().size()); +}