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

Closes #1161: Update make test-chapel to run unit tests #1561

Merged

Conversation

stress-tess
Copy link
Member

This PR (Closes #1161):

  • Updates make test-chapel to run unit tests in addition to compiling them
  • This now runs the same commands the CI uses i.e.
python3 src/serverModuleGen.py ServerModules.cfg
start_test test

NOTE:
I'm not the best with make, so please feel free to suggest a different approach. I got this working using .SILENT: but that feels like overkill. I think we really just need no-print-directory, but I'm not sure how to get that to work. Without the silent, make will exclaim everytime it switches into the test directory and that interupts the compile statement

Tagging @mhmerrill, @bmcdonald3, and @ronawho as they are better with make than me

This PR (Closes Bears-R-Us#1161):
- Updates `make test-chapel` to run unit tests in addition to compiling them
- This now calls the same commands the CI uses to run i.e.
```bash
python3 src/serverModuleGen.py ServerModules.cfg
start_test test
```
NOTE:
I'm not the best with `make`, so please feel free to suggest a different approach. I got this working using `.SILENT:` but that feels like overkill. I think we really just need `no-print-directory`, but I'm not sure how to get that to work. Without the silent, `make` will exclaim everytime it switches into the test directory and that interupts the compile statement
test-chapel: $(TEST_TARGETS)
.SILENT:
test-chapel:
@echo $(shell python3 $(MODULE_GENERATION_SCRIPT) $(ARKOUDA_CONFIG_FILE));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far as I know the chapel tests don't need the config file, why is this generation needed?

Copy link
Member Author

@stress-tess stress-tess Jul 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if they do. I basically was just following along with what we do for our testing in the CI

run: |
python3 src/serverModuleGen.py ServerModules.cfg
start_test test

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it was added in e622dc5 -- @bmcdonald3 can you take an action item to look into whether this is needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked into this; it turns out that CommandMap.chpl had an unnecessary use ServerRegistration for some reason that was causing the python script to be called first. I've opened #1575 to fix this.

Copy link
Contributor

@Ethan-DeBandi99 Ethan-DeBandi99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the line that @ronawho was saying may not be required will hurt anything by being there. I do want to note a few things to see if anyone else thinks they are an issue

  • The .SILENT appears to be causing the compile statements to disappear during make. Not sure if this is an issue, but may make debugging more of a pain.
  • I am seeing a failure during test/UnitTestParquetCpp.chpl. This is also happening for me on master. Good chance it may be due to my configuration, but wanted note in case anyone else is seeing it. Below is the output from the .tmp
timedexec: target program died with signal 11, without coredump

@Ethan-DeBandi99
Copy link
Contributor

I did a bit more digging on the issue I was seeing with the test/UnitTestParquetCpp.chpl. It appears as though there is a good chance that this may be due to the chipset. I know @jeichert60 has an arm64 Mac as well, so I am going to get him to run it and see if his has the same issue. If he does, I am tempted to say that there may be something between x86 and arm64 that are causing the error.

@Ethan-DeBandi99
Copy link
Contributor

I did a bit more digging on the issue I was seeing with the test/UnitTestParquetCpp.chpl. It appears as though there is a good chance that this may be due to the chipset. I know @jeichert60 has an arm64 Mac as well, so I am going to get him to run it and see if his has the same issue. If he does, I am tempted to say that there may be something between x86 and arm64 that are causing the error.

On Friday, I had Jim run the test as well and it failed there. Thus it appears that this is an issue with the arm64 chipset when running the parquet test. I will try to find some time this week to look into it, and will probably want to touch base with @bmcdonald3 since he wrote our current Parquet code.

Copy link
Contributor

@Ethan-DeBandi99 Ethan-DeBandi99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reviewing, the issue here does not prevent the tests from running and is specific to the arm64 processors.

@mhmerrill mhmerrill merged commit 4299d6b into Bears-R-Us:master Jul 11, 2022
@stress-tess stress-tess deleted the 1161_update_make_test_chapel branch July 20, 2022 16:32
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 this pull request may close these issues.

fix make test-chapel to not only compile arkouda-chapel unit tests but also run them
7 participants