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.

Sign up for GitHub

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

Jump to bottom

Added parSafe unstable warnings to Set and Map modules. #22772

Merged
merged 14 commits into from
Jul 20, 2023
Merged

Added parSafe unstable warnings to Set and Map modules. #22772

merged 14 commits into from
Jul 20, 2023

Conversation

Iainmon
Copy link
Contributor

@Iainmon Iainmon commented Jul 19, 2023 •
edited
Loading

Adding unstable flags for parSafe in Map and Set modules.

  • Check to see how the docs look
  • Run paratest

@Iainmon
Copy link
Contributor Author

Iainmon commented Jul 19, 2023

Is it normal to have a paratest result of [Summary: #Successes = 14225 | #Failures = 590 | #Futures = 871]?

jeremiah-corrado
jeremiah-corrado reviewed Jul 19, 2023
View reviewed changes
Copy link
Contributor

@jeremiah-corrado jeremiah-corrado left a comment

Choose a reason for hiding this comment

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

Looks good overall! Just a couple of comments:

test/unstable/Map/mapParSafe.chpl Outdated Show resolved Hide resolved
test/unstable/Set/setParSafe.chpl Outdated Show resolved Hide resolved
@jeremiah-corrado
Copy link
Contributor

Is it normal to have a paratest result of [Summary: #Successes = 14225 | #Failures = 590 | #Futures = 871]

No, you shouldn't be seeing that many failures for a change like this. It's probably an issue with the way you have your chapel environment configured. Let's discuss offline though.

@Iainmon
Merge added tests into existing tests
036122f 
Signed-off-by: Iain Moncrief <[email protected]>
@Iainmon
Added prediff files
dffc1eb 
Signed-off-by: Iain Moncrief <[email protected]>
@Iainmon
Added nnnn to good files
2e2e202 
Signed-off-by: Iain Moncrief <[email protected]>
@Iainmon
UnAdded nnnn to good files
55f63d1 
Signed-off-by: Iain Moncrief <[email protected]>
jeremiah-corrado
jeremiah-corrado reviewed Jul 20, 2023
View reviewed changes
test/unstable/Map/parSafe.prediff Outdated
sed -E 's/\.chpl:[0-9]*:/\.chpl:nnnn:/' < $OUTFILE > $TMPFILE
cat $TMPFILE > $OUTFILE

rm $TMPFILE
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure you add a newline at the end of this file.

(There is a VS code extension called "NewLine" that does this for you automatically whenever you save a file)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, in my offline message, I was suggesting that you add these .prediff files to types/chplhashtable/noHashForRecordWithEq and types/chplhashtable/noHashForRecordWithEq2 to prevent the test failures you were seeing.

You don't need them here since the line numbers are coming from the test itself rather than an internal module file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohhh, I see. Thank you for the clarification

jeremiah-corrado
jeremiah-corrado reviewed Jul 20, 2023
View reviewed changes
test/types/chplhashtable/noHashForRecordWithEq.good Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you still need to update the line numbers here to nnnn to get these tests to pass. If they are passing as is, then that means start_test isn't invoking the .prediff file and you'll probably need to chmod +x them.

jeremiah-corrado
jeremiah-corrado approved these changes Jul 20, 2023
View reviewed changes
Copy link
Contributor

@jeremiah-corrado jeremiah-corrado left a comment

Choose a reason for hiding this comment

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

Looks good! Just one addition to the tests that would be good to see:

test/unstable/Map/parSafe.chpl Outdated Show resolved Hide resolved
test/unstable/Set/parSafe.chpl Outdated Show resolved Hide resolved
@Iainmon
Copy link
Contributor Author

Iainmon commented Jul 20, 2023

Ran start_test in both test/unstable/{Map,Set} directories.

@Iainmon Iainmon merged commit 06ebd23 into chapel-lang:main Jul 20, 2023
7 checks passed
@jeremiah-corrado jeremiah-corrado mentioned this pull request Jul 21, 2023
Merged
1 task
jeremiah-corrado added a commit that referenced this pull request Jul 21, 2023
@jeremiah-corrado
Test fixes from unstabilizing set.parSafe and map.parSafe (#22792)
9ed82f0 
Fixes a few test failures from
#22772:

- `compflags/TestWarnUnstableSuppression`
- `types/chplhashtable/noHashForRecordWithEq`
- `types/chplhashtable/noHashForRecordWithEq2`

[ trivial - not reviewed ]

- [x] paratest
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeremiah-corrado jeremiah-corrado jeremiah-corrado approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Iainmon @jeremiah-corrado