Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CI #13

Closed
vil02 opened this issue Sep 12, 2024 · 9 comments · Fixed by #18
Closed

Add CI #13

vil02 opened this issue Sep 12, 2024 · 9 comments · Fixed by #18
Assignees

Comments

@vil02
Copy link
Member

vil02 commented Sep 12, 2024

This repository needs some CI. Before adding a corresponding workflow, I would like to understand what is the intended structure. The contributing guidelines suggest to use modules, but currently there is only one module: merge_sort.f90.

Would it make sense to have the directory structure like:

.
├── examples
│   ├── maths
│   │   ├── example_euclid_gcd.f90
│   │   └── example_fibonacci.f90
│   └── sorts
│       ├── example_bubble_sort.f90
│       └── example_recursive_bubble_sort.f90
├── modules
│   ├── maths
│   │   ├── euclid_gcd.f90
│   │   ├── factorial.f90
│   │   └── fibonacci.f90
│   └── sorts
│       ├── bubble_sort.f90
│       ├── example_usage_merge_sort.f90
│       ├── merge_sort.f90
│       └── recursive_bubble_sort.f90
└── tests
    ├── maths
    │   ├── test_euclid_gcd.f90
    │   ├── test_factorial.f90
    │   └── test_fibonacci.f90
    └── sorts
        ├── test_bubble_sort.f90
        ├── test_merge_sort.f90
        └── test_recursive_bubble_sort.f90

where:

  • every file from modules has a corresponding test_*.f90 file in tests,
  • some of the files in modules might have a corresponding example_*.f90 file in examples.
  • the files in tests are the basic unit tests for given module,
  • the files in examples should also have assert statements, but they should not be included into the coverage reports.

The currently existing files, which are not modules could be temporary moved to a directory like other and moved over time to the correct place.

@SatinWukerORIG if you agree on such approach, feel free to assign me to this issue.

@SatinWukerORIG
Copy link
Member

Hi @vil02
Sorry for the late reply!
After discussing with @Ramy-Badr-Ahmed , I think this structure is suitable for this repo. Thank you 😊
Here, I will assign you to this issue.

@Ramy-Badr-Ahmed
Copy link
Member

Ramy-Badr-Ahmed commented Sep 16, 2024

the files in tests are the basic unit tests for given module,

How about using pFUnit for the unit tests?

It provides a more formal framework for unit testing, including assertions, test suites, and detailed reporting, much like popular testing frameworks in other languages (e.g., JUnit for Java, pytest for Python)

https://github.com/Goddard-Fortran-Ecosystem/pFUnit

@SatinWukerORIG
Copy link
Member

@Ramy-Badr-Ahmed I am not sure here... I think pFUnit should not be a requirement for Fortran Contributors. Our goal is to keep everything simple to every Fortran algorithm learner. Requiring pFUnit increases people's cost of learning. Also, if a learner struggles with installing pFUnit in their environment, then he or she is not able to contribute, and I don't want that to happen.
However, pFUnit can be included to the code.
@vil02 What do you think about this?

@Ramy-Badr-Ahmed
Copy link
Member

@Ramy-Badr-Ahmed I am not sure here... I think pFUnit should not be a requirement for Fortran Contributors. Our goal is to keep everything simple to every Fortran algorithm learner. Requiring pFUnit increases people's cost of learning. Also, if a learner struggles with installing pFUnit in their environment, then he or she is not able to contribute, and I don't want that to happen. However, pFUnit can be included to the code. @vil02 What do you think about this?

Good point! it can probably be mentioned like an option in the contribution guidelines.

@vil02
Copy link
Member Author

vil02 commented Sep 16, 2024

@Ramy-Badr-Ahmed I am not sure here... I think pFUnit should not be a requirement for Fortran Contributors. Our goal is to keep everything simple to every Fortran algorithm learner. Requiring pFUnit increases people's cost of learning. Also, if a learner struggles with installing pFUnit in their environment, then he or she is not able to contribute, and I don't want that to happen. However, pFUnit can be included to the code. @vil02 What do you think about this?

At the moment the things in tests do not test anything - they just display stuff.

Using some testing framework is always a good idea. I would suggest to use something standard - not only to have some old school FORTRAN vibe but because of the fact that people learning FORTRAN today most likely will work with some legacy stuffy.

Which one to use? I have no idea... I do not have enough knowledge about FORTRAN. The most important thing is to make these tests actually check something, even using plain FORTRAN.

Regarding the CI: I think I will setup this project using CMake - @Ramy-Badr-Ahmed, @SatinWukerORIG any thoughts on that?

@SatinWukerORIG
Copy link
Member

Ok, we should go with pFTest then, unless I find a better alternative within 2 days lol. Regarding CI, CMake is great!

@Ramy-Badr-Ahmed
Copy link
Member

@Ramy-Badr-Ahmed I am not sure here... I think pFUnit should not be a requirement for Fortran Contributors. Our goal is to keep everything simple to every Fortran algorithm learner. Requiring pFUnit increases people's cost of learning. Also, if a learner struggles with installing pFUnit in their environment, then he or she is not able to contribute, and I don't want that to happen. However, pFUnit can be included to the code. @vil02 What do you think about this?

At the moment the things in tests do not test anything - they just display stuff.

Using some testing framework is always a good idea. I would suggest to use something standard - not only to have some old school FORTRAN vibe but because of the fact that people learning FORTRAN today most likely will work with some legacy stuffy.

Which one to use? I have no idea... I do not have enough knowledge about FORTRAN. The most important thing is to make these tests actually check something, even using plain FORTRAN.

Regarding the CI: I think I will setup this project using CMake - @Ramy-Badr-Ahmed, @SatinWukerORIG any thoughts on that?

Indeed, the current tests are set up to display the original and sorted arrays for manual visual inspection. It’s a good idea to automate these checks using a testing framework. I agree that using a standard framework would be beneficial for consistency and practicality. However, setting up the pFUnit environment might not be straightforward and could require some additional configuration.

CMake is a suitable choice for setting up CI, and I’m currently using it in CLion. It should work well for managing the build and testing process.

@SatinWukerORIG , for your reference: https://fortran-lang.discourse.group/t/unit-testing-frameworks-for-fortran/6944/4

@Ramy-Badr-Ahmed
Copy link
Member

Ramy-Badr-Ahmed commented Sep 16, 2024

@vil02

I suggest the following CI as startring point with CMake:

# .github/workflows/ci.yml
name: CI

on:
  push:
    branches: [ main ] 
  pull_request:
    branches: [ main ]  

jobs:
  build:
    runs-on: debian-latest 

    steps:
    - name: Checkout code
      uses: actions/checkout@v3

    - name: Set up CMake
      uses: cmake/setup-cmake@v3
      with:
        cmake-version: '3.29'

    - name: Install dependencies
      run: sudo apt-get install -y gfortran

    - name: Configure CMake
      run: cmake -S . -B build

    - name: Build
      run: cmake --build build

    - name: Run tests
      run: cmake --build build --target test

Including pFUnit or other dependencies can be added the Install dependencies step.

@Ramy-Badr-Ahmed
Copy link
Member

@SatinWukerORIG , @vil02

The official Fortran community on GitHub offers test-drive for testing and fprettify for code styling. Perhaps it can be integrated into the CI?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants