Re: [PATCH olang 2/2] tests: add integration test setup

[Johnny Richard <johnny@johnnyrichard.com>]

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 <carlos@maniero.me>
> ---
>  .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 <https://www.gnu.org/licenses/>.
> + */
> +#include "cli_runner.h"
> +#include <assert.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +#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 <https://www.gnu.org/licenses/>.
> + */
> +#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?