[mlir-lsp] Un-revert unit test additions #90232
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts the revert commit 6844c2f, which was comprised of the following commits: 1. f3f6f22 - [mlir-lsp] Initialize `Reply::method` member (llvm#89857) 2. 37e13d4 - [mlir-lsp] Log invalid notification params (llvm#89856) 3. ba1b52e - [mlir-lsp] Add `outgoingNotification` unit test 4. 84bc21f - [mlir-lsp] Add transport unit tests (llvm#89855) Of these, (4) specifically caused issues in Windows pre-merge buildbots, in the `TransportTest.MethodNotFound` unit test that it added. The failure was caused by a statement that asserted that opening a file stream on a newly created temporary file did not result in an error, but this assert failed on Windows. This patch adds additional error logging for failures, to make it clearer what went wrong when failures occur. This patch also addresses the Windows failure by ensuring temporary files are created in the system temporary directory.
|
@llvm/pr-subscribers-mlir-core @llvm/pr-subscribers-mlir Author: Brian Gesiak (modocache) ChangesThis reverts the revert commit 6844c2f, which was comprised of the following commits:
Of these, (4) specifically caused issues in Windows pre-merge buildbots, in the This patch adds additional error logging for failures, to make it clearer what went wrong when failures occur. This patch also addresses the Windows failure by ensuring temporary files are created in the system temporary directory. Full diff: https://github.com/llvm/llvm-project/pull/90232.diff 6 Files Affected:
diff --git a/mlir/include/mlir/Tools/lsp-server-support/Transport.h b/mlir/include/mlir/Tools/lsp-server-support/Transport.h
index ce742be7a941c9..44c71058cf717c 100644
--- a/mlir/include/mlir/Tools/lsp-server-support/Transport.h
+++ b/mlir/include/mlir/Tools/lsp-server-support/Transport.h
@@ -147,9 +147,15 @@ class MessageHandler {
void (ThisT::*handler)(const Param &)) {
notificationHandlers[method] = [method, handler,
thisPtr](llvm::json::Value rawParams) {
- llvm::Expected<Param> param = parse<Param>(rawParams, method, "request");
- if (!param)
- return llvm::consumeError(param.takeError());
+ llvm::Expected<Param> param =
+ parse<Param>(rawParams, method, "notification");
+ if (!param) {
+ return llvm::consumeError(
+ llvm::handleErrors(param.takeError(), [](const LSPError &lspError) {
+ Logger::error("JSON parsing error: {0}",
+ lspError.message.c_str());
+ }));
+ }
(thisPtr->*handler)(*param);
};
}
diff --git a/mlir/lib/Tools/lsp-server-support/Transport.cpp b/mlir/lib/Tools/lsp-server-support/Transport.cpp
index 64dea35614c070..339c5f3825165d 100644
--- a/mlir/lib/Tools/lsp-server-support/Transport.cpp
+++ b/mlir/lib/Tools/lsp-server-support/Transport.cpp
@@ -51,12 +51,12 @@ class Reply {
Reply::Reply(const llvm::json::Value &id, llvm::StringRef method,
JSONTransport &transport, std::mutex &transportOutputMutex)
- : id(id), transport(&transport),
+ : method(method), id(id), transport(&transport),
transportOutputMutex(transportOutputMutex) {}
Reply::Reply(Reply &&other)
- : replied(other.replied.load()), id(std::move(other.id)),
- transport(other.transport),
+ : method(other.method), replied(other.replied.load()),
+ id(std::move(other.id)), transport(other.transport),
transportOutputMutex(other.transportOutputMutex) {
other.transport = nullptr;
}
diff --git a/mlir/unittests/CMakeLists.txt b/mlir/unittests/CMakeLists.txt
index 6fad249a0b2fba..6d8aa290e82f25 100644
--- a/mlir/unittests/CMakeLists.txt
+++ b/mlir/unittests/CMakeLists.txt
@@ -20,6 +20,7 @@ add_subdirectory(Support)
add_subdirectory(Rewrite)
add_subdirectory(TableGen)
add_subdirectory(Target)
+add_subdirectory(Tools)
add_subdirectory(Transforms)
if(MLIR_ENABLE_EXECUTION_ENGINE)
diff --git a/mlir/unittests/Tools/CMakeLists.txt b/mlir/unittests/Tools/CMakeLists.txt
new file mode 100644
index 00000000000000..a97588d9286685
--- /dev/null
+++ b/mlir/unittests/Tools/CMakeLists.txt
@@ -0,0 +1 @@
+add_subdirectory(lsp-server-support)
diff --git a/mlir/unittests/Tools/lsp-server-support/CMakeLists.txt b/mlir/unittests/Tools/lsp-server-support/CMakeLists.txt
new file mode 100644
index 00000000000000..3aa8b9c4bc7735
--- /dev/null
+++ b/mlir/unittests/Tools/lsp-server-support/CMakeLists.txt
@@ -0,0 +1,6 @@
+add_mlir_unittest(MLIRLspServerSupportTests
+ Transport.cpp
+)
+target_link_libraries(MLIRLspServerSupportTests
+ PRIVATE
+ MLIRLspServerSupportLib)
diff --git a/mlir/unittests/Tools/lsp-server-support/Transport.cpp b/mlir/unittests/Tools/lsp-server-support/Transport.cpp
new file mode 100644
index 00000000000000..a086964cd3660c
--- /dev/null
+++ b/mlir/unittests/Tools/lsp-server-support/Transport.cpp
@@ -0,0 +1,134 @@
+//===- Transport.cpp - LSP JSON transport unit tests ----------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "mlir/Tools/lsp-server-support/Transport.h"
+#include "mlir/Tools/lsp-server-support/Logging.h"
+#include "mlir/Tools/lsp-server-support/Protocol.h"
+#include "llvm/Support/FileSystem.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace mlir;
+using namespace mlir::lsp;
+using namespace testing;
+
+namespace {
+
+TEST(TransportTest, SendReply) {
+ std::string out;
+ llvm::raw_string_ostream os(out);
+ JSONTransport transport(nullptr, os);
+ MessageHandler handler(transport);
+
+ transport.reply(1989, nullptr);
+ EXPECT_THAT(out, HasSubstr("\"id\":1989"));
+ EXPECT_THAT(out, HasSubstr("\"result\":null"));
+}
+
+class TransportInputTest : public Test {
+ llvm::SmallVector<char> inputPath;
+ std::FILE *in = nullptr;
+ std::string output = "";
+ llvm::raw_string_ostream os;
+ std::optional<JSONTransport> transport = std::nullopt;
+ std::optional<MessageHandler> messageHandler = std::nullopt;
+
+protected:
+ TransportInputTest() : os(output) {}
+
+ void SetUp() override {
+ std::error_code ec =
+ llvm::sys::fs::createTemporaryFile("lsp-unittest", "json", inputPath);
+ ASSERT_FALSE(ec) << "Could not create temporary file: " << ec.message();
+
+ in = std::fopen(inputPath.data(), "r");
+ ASSERT_TRUE(in) << "Could not open temporary file: "
+ << std::strerror(errno);
+ transport.emplace(in, os, JSONStreamStyle::Delimited);
+ messageHandler.emplace(*transport);
+ }
+
+ void TearDown() override {
+ EXPECT_EQ(std::fclose(in), 0)
+ << "Could not close temporary file FD: " << std::strerror(errno);
+ std::error_code ec =
+ llvm::sys::fs::remove(inputPath, /*IgnoreNonExisting=*/false);
+ EXPECT_FALSE(ec) << "Could not remove temporary file '" << inputPath.data()
+ << "': " << ec.message();
+ }
+
+ void writeInput(StringRef buffer) {
+ std::error_code ec;
+ llvm::raw_fd_ostream os(inputPath.data(), ec);
+ ASSERT_FALSE(ec) << "Could not write to '" << inputPath.data()
+ << "': " << ec.message();
+ os << buffer;
+ os.close();
+ }
+
+ StringRef getOutput() const { return output; }
+ MessageHandler &getMessageHandler() { return *messageHandler; }
+
+ void runTransport() {
+ bool gotEOF = false;
+ llvm::Error err = llvm::handleErrors(
+ transport->run(*messageHandler), [&](const llvm::ECError &ecErr) {
+ gotEOF = ecErr.convertToErrorCode() == std::errc::io_error;
+ });
+ llvm::consumeError(std::move(err));
+ EXPECT_TRUE(gotEOF);
+ }
+};
+
+TEST_F(TransportInputTest, RequestWithInvalidParams) {
+ struct Handler {
+ void onMethod(const TextDocumentItem ¶ms,
+ mlir::lsp::Callback<TextDocumentIdentifier> callback) {}
+ } handler;
+ getMessageHandler().method("invalid-params-request", &handler,
+ &Handler::onMethod);
+
+ writeInput("{\"jsonrpc\":\"2.0\",\"id\":92,"
+ "\"method\":\"invalid-params-request\",\"params\":{}}\n");
+ runTransport();
+ EXPECT_THAT(getOutput(), HasSubstr("error"));
+ EXPECT_THAT(getOutput(), HasSubstr("missing value at (root).uri"));
+}
+
+TEST_F(TransportInputTest, NotificationWithInvalidParams) {
+ // JSON parsing errors are only reported via error logging. As a result, this
+ // test can't make any expectations -- but it prints the output anyway, by way
+ // of demonstration.
+ Logger::setLogLevel(Logger::Level::Error);
+
+ struct Handler {
+ void onNotification(const TextDocumentItem ¶ms) {}
+ } handler;
+ getMessageHandler().notification("invalid-params-notification", &handler,
+ &Handler::onNotification);
+
+ writeInput("{\"jsonrpc\":\"2.0\",\"method\":\"invalid-params-notification\","
+ "\"params\":{}}\n");
+ runTransport();
+}
+
+TEST_F(TransportInputTest, MethodNotFound) {
+ writeInput("{\"jsonrpc\":\"2.0\",\"id\":29,\"method\":\"ack\"}\n");
+ runTransport();
+ EXPECT_THAT(getOutput(), HasSubstr("\"id\":29"));
+ EXPECT_THAT(getOutput(), HasSubstr("\"error\""));
+ EXPECT_THAT(getOutput(), HasSubstr("\"message\":\"method not found: ack\""));
+}
+
+TEST_F(TransportInputTest, OutgoingNotification) {
+ auto notifyFn = getMessageHandler().outgoingNotification<CompletionList>(
+ "outgoing-notification");
+ notifyFn(CompletionList{});
+ EXPECT_THAT(getOutput(), HasSubstr("\"method\":\"outgoing-notification\""));
+}
+} // namespace
|
|
Note: I didn't add reviewers to this pull request for now; I'll wait until I get green pre-merge builds that confirm the fix is good. |
|
The Windows pre-merge buildbot shows a successful run of the new tests -- although, it did take 27 hours for the bot to launch. |
walter-erquinigo
approved these changes
Apr 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This reverts the revert commit 6844c2f, which was comprised of the following commits:
Reply::methodmember ([mlir-lsp] Initialize Reply::method member #89857)outgoingNotificationunit testOf these, (4) specifically caused issues in Windows pre-merge buildbots, in the
TransportTest.MethodNotFoundunit test that it added. The failure was caused by a statement that asserted that opening a file stream on a newly created temporary file did not result in an error, but this assert failed on Windows.This patch adds additional error logging for failures, to make it clearer what went wrong when failures occur. This patch also addresses the Windows failure by ensuring temporary files are created in the system temporary directory.