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

test: improve exception serialization test #2962

Closed
wants to merge 11 commits into from

Conversation

boscohyun
Copy link
Member

@boscohyun boscohyun commented Oct 29, 2024

This pull request need to wait until the ExceptionFormatter fixed.

@boscohyun boscohyun requested review from a team October 29, 2024 10:43
@boscohyun boscohyun self-assigned this Oct 29, 2024
@boscohyun
Copy link
Member Author

This pull request need to rebase after #2963 merged.

@boscohyun
Copy link
Member Author

Rebase needed after #2975 merged, and remove ignore cases of exceptions test.

Copy link

cloudflare-workers-and-pages bot commented Nov 5, 2024

Deploying lib9c with  Cloudflare Pages  Cloudflare Pages

Latest commit: 0543ab3
Status:🚫  Build failed.

View logs

Comment on lines +52 to +59
if (e == typeof(InvalidBlockProtocolVersionException) ||
e == typeof(InvalidBlockStateRootHashException) ||
e == typeof(DuplicateVoteException))
{
// FIXME:
// MessagePack.MessagePackSerializationException: Failed to serialize System.Exception value.
continue;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

@ipdae @eugene-doobu 기존 테스트에서 모든 Libplanet 예외도 테스트하게 수정했어요. 그런데 이 세 예외만 같은 에러 메시지와 함께 테스트에서 실패하고 있어요.

MessagePack.MessagePackSerializationException: Failed to serialize System.Exception value.

@eugene-doobu eugene-doobu self-requested a review November 20, 2024 02:02
e == typeof(InvalidBlockStateRootHashException) ||
e == typeof(DuplicateVoteException))
{
// FIXME:
Copy link
Member

Choose a reason for hiding this comment

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

해당 fixme주석까지 해결 후 머지하려고 생각했는데 계속 리뷰가 밀리고 있어 죄송합니다.
일단 구현해주신 상태로 머지 후 해당 케이스들은 별도 이슈로 만들어 처리하려고 하는데 괜찮으실까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

네 좋아요. 지금 CI 실패하는 것은 제가 바로 볼게요.

Copy link
Member

Choose a reason for hiding this comment

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

   System.InvalidOperationException : No suitable constructor found for Libplanet.KeyStore.NoKeyException.

위 실패하는 exception이 에디터에서도 실패하는지 보려고 테스트를 돌려봤는데, 에디터에서는 모든 테스트가 통과하고 위 예외에 대한 테스트는 진행하지 않은 것 같네요. ci 환경과 뭔가 설정 차이가 있나 싶습니다

Copy link
Member Author

Choose a reason for hiding this comment

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

이번에 libplanet 5.4.0 버전을 범프하면서 예외가 추가됐거나 뭔가 수정 사항이 있나봐요. 일단 좀 고쳐봤고, 이거 통과되면 머지할게요.
이슈도 만들어뒀어요: #3025

@boscohyun
Copy link
Member Author

I try re-open this pull request to fix "license/cla Expected — Waiting for status to be reported".

@eugene-doobu eugene-doobu deleted the test/improve-exception-test branch November 20, 2024 04:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

handle all of exception types in Lib9c.Tests.ExceptionTest.Exception_Serializable test
2 participants