From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mail-a.sr.ht; dkim=pass header.d=johnnyrichard.com header.i=@johnnyrichard.com Received: from out-174.mta0.migadu.com (out-174.mta0.migadu.com [91.218.175.174]) by mail-a.sr.ht (Postfix) with ESMTPS id 458A5200EF for <~johnnyrichard/olang-devel@lists.sr.ht>; Thu, 15 Feb 2024 21:18:58 +0000 (UTC) Date: Thu, 15 Feb 2024 23:21:00 +0100 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=johnnyrichard.com; s=key1; t=1708031936; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=Cs9re9cyV1rzgD/ZGtE1XvRZW5WPedFxM+jShoNGEBc=; b=ii0BhnQ6fs30xPtZhXGEnqna5GW956GsnxW5Rr6bodd4wJkuBdLIkS7Xe+lMRakY8CTP8P EfyR566LsqhjZns3JNfStpnY26o6yyJwfjrj54I46oUMH3Q6xR8fcvvBmSgO5Z0wN5ZWqx H5LVicCzcITvd0cxsT5Z11zoPcuODGCjgGuTsp1UvEuBM90/r2u0vIzXVa+fLw5T1J1qoS gtwyrhJrTGu15A6cf2Yv/b2uyzPtHZWoa+CvrK+PBNPW29ionQKzQ6UiihWRE8E/NGeKpB p7t1ZXIA2IS8nDzqnrTWr6tRP+qnBDnzJxL/DxNIEctN8LJgwphy2Yh1b0F4Hw== X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Johnny Richard To: Carlos Maniero Cc: ~johnnyrichard/olang-devel@lists.sr.ht Subject: Re: [PATCH olang 2/2] tests: add integration test setup Message-ID: <2fltgo6fnzgcnrmvi5akr7d5kc7qkrfsxaketlsdet4my5i6c2@hfozssfoes3o> X-Sourcehut-Patchset-Update: NEEDS_REVISION References: <20240215162146.847336-1-carlos@maniero.me> <20240215162146.847336-3-carlos@maniero.me> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240215162146.847336-3-carlos@maniero.me> X-Migadu-Flow: FLOW_OUT X-TUID: aTa+GbWT8WO8 In general it looks good, no big issues at all. I just would like to discuss few topics and see your point of view. On Thu, Feb 15, 2024 at 01:21:46PM -0300, Carlos Maniero wrote: > This is a basic setup for integration tests which includes: > > - a *cli_runner* helper file to provide an interface to interact with > the CLI. > - a simple test that validates if the compiler is returning zero status nitpick: s/status/exit/ > code when a file is provided. > > At this point the compiler still doing nothing making all this bootstrap > just a fancy way to check if the compiler was compiled properly. > > Signed-off-by: Carlos Maniero > --- > .build.yml | 6 ++- > Makefile | 16 ++++++++ > tests/integration/Makefile | 27 +++++++++++++ > tests/integration/cli_runner.c | 71 ++++++++++++++++++++++++++++++++++ > tests/integration/cli_runner.h | 27 +++++++++++++ > tests/integration/cli_test.c | 39 +++++++++++++++++++ What is the motivation behind segregate integration tests from unit tests? > diff --git a/Makefile b/Makefile > index 2a23b59..fa3df6c 100644 > --- a/Makefile > +++ b/Makefile > @@ -20,9 +20,25 @@ $(BUILD_DIR): > linter: $(SRCS) $(HEADERS) > clang-format --dry-run --Werror $? > > +.PHONY: linter-all > +linter-all: $(SRCS) $(HEADERS) > + make linter nitpick: Let's use $(MAKE) instead of make directly. > + make -C tests/integration/ linter > + > + > .PHONY: linter-fix > linter-fix: $(SRCS) $(HEADERS) > clang-format -i $? > > +.PHONY: linter-fix-all > +linter-fix-all: $(SRCS) $(HEADERS) What do you think about the /linter-fix/ and /linter/ scan all files instead of introducing new targets with -all suffix? > + make linter-fix > + make -C tests/integration/ linter-fix > + > +.PHONY: integration-test > +integration-test: What do you think about adding a /check/ target that runs all tests on the project. > diff --git a/tests/integration/cli_runner.c b/tests/integration/cli_runner.c > new file mode 100644 > index 0000000..cd7ba22 > --- /dev/null > +++ b/tests/integration/cli_runner.c > @@ -0,0 +1,71 @@ > +/* > + * Copyright (C) 2024 olang maintainers > + * > + * This program is free software: you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation, either version 3 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see . > + */ > +#include "cli_runner.h" > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define OLANG_COMPILER_PATH "../../0c" > + > +int _compiler_exists_already_checked = 0; nitpick: I cannot see a big benefit of adding an underscore for this property neither for methods. I understand you want to communicate that it should not be access by other sources. What do you think about making these "_" properties and functions static? > + > +void > +_assert_compiler_exists() > +{ > + { > + if (_compiler_exists_already_checked == 1) { > + return; > + } > + > + _compiler_exists_already_checked = 1; > + } > + > + FILE *file; > + if ((file = fopen(OLANG_COMPILER_PATH, "r"))) { suggestion: We could be more clear what is under test like bellow: FILE *file = file = fopen(OLANG_COMPILER_PATH, "r"); if (file != NULL) { > + fclose(file); > + return; > + } > + > + assert(false && "Compiler not found. Build the compiler before executing tests."); nitpick: We could use a /perror/ followed by a /exit/ to be more descriptive on the message error. perror("Compiler not found. Build the compiler before executing tests."); exit(EXIT_FAILURE); What do you think? > +} > + > +void > +_create_tmp_file_name(char *file_name) > +{ > + sprintf(file_name, "%s/olang_programXXXXXX", P_tmpdir); > + int fd = mkstemp(file_name); > + assert(fd != -1 && "Could not create a tmp file"); suggestion: Could we also use /perror/ and /exit/ here as well? > diff --git a/tests/integration/cli_runner.h b/tests/integration/cli_runner.h > new file mode 100644 > index 0000000..0df7f2d > --- /dev/null > +++ b/tests/integration/cli_runner.h > @@ -0,0 +1,27 @@ > +/* > + * Copyright (C) 2024 olang maintainers > + * > + * This program is free software: you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation, either version 3 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see . > + */ > +#ifndef CLI_RUNNER_H > +#define CLI_RUNNER_H > +typedef struct cli_result_t > +{ > + int exit_code; > + char binary_loc[255]; What is binary_loc? Does it means location or path?