PLT-5551 Unit Tests [PLT-5867 + PLT-5868] #76
Conversation
|
There are some failures even at this point, but I would recommend to review the setup of the tests, and the way they are written for the time-being. As, most of them will definitely have to be rewritten once the revamp is in place. |
|
Walkthrough This pull request introduces a variety of changes across multiple files, primarily focusing on enhancing unit tests, improving thread safety in Python code, and refining the Plutus Certification module. The updates include adding Changes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| "Cardano/Crypto/Encoding/Seed" | ||
| "Crypto/Math/Edwards25519" | ||
| "Crypto/Math/Bits" | ||
| "Crypto/Math/Bytes" | ||
| "Crypto/Math/NatMath" | ||
| "Crypto/ECC/Ed25519Donna" | ||
| "Crypto/ECC/Ed25519BIP32" | ||
| "Crypto/Encoding/BIP39" | ||
| "Crypto/Encoding/BIP39/Dictionary" | ||
| "Crypto/Encoding/BIP39/English" | ||
| "Cardano/Internal/Compat" | ||
| ]; | ||
| cSources = [ "cbits/ed25519/ed25519.c" "cbits/encrypted_sign.c" ]; | ||
| hsSourceDirs = [ "src" ]; | ||
| includeDirs = [ "cbits/ed25519" "cbits" ]; | ||
| }; | ||
| exes = { | ||
| "golden-tests" = { | ||
| depends = [ | ||
| (hsPkgs."base" or (errorHandler.buildDepError "base")) | ||
| (hsPkgs."basement" or (errorHandler.buildDepError "basement")) | ||
| (hsPkgs."foundation" or (errorHandler.buildDepError "foundation")) | ||
| (hsPkgs."memory" or (errorHandler.buildDepError "memory")) | ||
| (hsPkgs."bytestring" or (errorHandler.buildDepError "bytestring")) | ||
| (hsPkgs."cryptonite" or (errorHandler.buildDepError "cryptonite")) | ||
| (hsPkgs."cardano-crypto" or (errorHandler.buildDepError "cardano-crypto")) | ||
| ] ++ (pkgs.lib).optional (flags.golden-tests-exe) (hsPkgs."inspector" or (errorHandler.buildDepError "inspector")); | ||
| buildable = if flags.golden-tests-exe then true else false; | ||
| modules = [ "Test/Orphans" ]; | ||
| hsSourceDirs = [ "test" ]; | ||
| mainPath = [ "GoldenTest.hs" ] ++ [ "" ]; | ||
| }; | ||
| }; | ||
| tests = { | ||
| "cardano-crypto-test" = { | ||
| depends = [ | ||
| (hsPkgs."base" or (errorHandler.buildDepError "base")) | ||
| (hsPkgs."bytestring" or (errorHandler.buildDepError "bytestring")) | ||
| (hsPkgs."memory" or (errorHandler.buildDepError "memory")) | ||
| (hsPkgs."cryptonite" or (errorHandler.buildDepError "cryptonite")) | ||
| (hsPkgs."cardano-crypto" or (errorHandler.buildDepError "cardano-crypto")) | ||
| (hsPkgs."basement" or (errorHandler.buildDepError "basement")) | ||
| (hsPkgs."foundation" or (errorHandler.buildDepError "foundation")) | ||
| ]; | ||
| buildable = true; | ||
| modules = [ | ||
| "Test/Crypto" | ||
| "Test/Crypto/Encoding" | ||
| "Test/Crypto/Encoding/BIP39" | ||
| "Test/Cardano" | ||
| "Test/Cardano/Crypto" | ||
| "Test/Cardano/Crypto/Encoding" | ||
| "Test/Cardano/Crypto/Encoding/Seed" | ||
| "Utils" | ||
| ]; | ||
| hsSourceDirs = [ "test" ]; | ||
| mainPath = [ "Spec.hs" ]; | ||
| }; | ||
| "cardano-crypto-golden-tests" = { | ||
| depends = [ | ||
| (hsPkgs."base" or (errorHandler.buildDepError "base")) | ||
| (hsPkgs."basement" or (errorHandler.buildDepError "basement")) | ||
| (hsPkgs."foundation" or (errorHandler.buildDepError "foundation")) | ||
| (hsPkgs."memory" or (errorHandler.buildDepError "memory")) | ||
| (hsPkgs."bytestring" or (errorHandler.buildDepError "bytestring")) | ||
| (hsPkgs."cryptonite" or (errorHandler.buildDepError "cryptonite")) | ||
| (hsPkgs."cardano-crypto" or (errorHandler.buildDepError "cardano-crypto")) | ||
| ] ++ (pkgs.lib).optional (flags.golden-tests) (hsPkgs."inspector" or (errorHandler.buildDepError "inspector")); | ||
| buildable = if flags.golden-tests then true else false; | ||
| modules = [ "Test/Orphans" ]; | ||
| hsSourceDirs = [ "test" ]; | ||
| mainPath = [ "GoldenTest.hs" ]; | ||
| }; | ||
| }; | ||
| benchmarks = { | ||
| "cardano-crypto-bench" = { | ||
| depends = [ | ||
| (hsPkgs."base" or (errorHandler.buildDepError "base")) | ||
| (hsPkgs."bytestring" or (errorHandler.buildDepError "bytestring")) | ||
| (hsPkgs."memory" or (errorHandler.buildDepError "memory")) | ||
| (hsPkgs."cryptonite" or (errorHandler.buildDepError "cryptonite")) | ||
| (hsPkgs."cardano-crypto" or (errorHandler.buildDepError "cardano-crypto")) | ||
| (hsPkgs."gauge" or (errorHandler.buildDepError "gauge")) | ||
| ]; | ||
| buildable = true; | ||
| hsSourceDirs = [ "benchs" ]; | ||
| }; | ||
| }; | ||
| }; | ||
| } // { | ||
| src = (pkgs.lib).mkDefault (pkgs.fetchgit { | ||
| url = "0"; | ||
| rev = "minimal"; | ||
| sha256 = ""; | ||
| }) // { | ||
| url = "0"; | ||
| rev = "minimal"; | ||
| sha256 = ""; | ||
| }; | ||
| } |
There was a problem hiding this comment.
The url, rev, and sha256 fields in the src section (lines 143-150) are set to placeholder values. These should be updated with the correct repository URL, revision, and SHA-256 hash of the desired commit for the cardano-crypto package.
- url = "0";
- rev = "minimal";
- sha256 = "";
+ url = "<correct-repo-url>";
+ rev = "<correct-commit-hash>";
+ sha256 = "<correct-sha256-hash>";
| data Status = Queued | Failed | Succeeded | ReadyForCertification | Certified | Aborted | ||
| deriving (Show, Read, Bounded, Enum, Eq, Generic) | ||
|
|
||
| instance ToJSON Status where | ||
| toJSON :: Status -> Value | ||
| toJSON Queued = toJSON ("queued" :: Text) | ||
| toJSON Failed = toJSON ("failed" :: Text) | ||
| toJSON Succeeded = toJSON ("succeeded" :: Text) | ||
| toJSON ReadyForCertification = toJSON ("ready-for-certification" :: Text) | ||
| toJSON Certified = toJSON ("certified" :: Text) | ||
| toJSON Aborted = toJSON ("aborted" :: Text) | ||
|
|
||
| instance FromJSON Status where | ||
| parseJSON = | ||
| withText "Status" handle | ||
| where | ||
| handle "queued" = pure Queued | ||
| handle "failed" = pure Failed | ||
| handle "succeeded" = pure Succeeded | ||
| handle "certified" = pure Succeeded | ||
| handle "ready-for-certification" = pure ReadyForCertification | ||
| handle "aborted" = pure Aborted | ||
| handle t = fail $ "provided text (" ++ show t ++ ") is not a Status" |
There was a problem hiding this comment.
The Status enum and its JSON serialization/deserialization methods are well implemented. However, there seems to be a mistake in the FromJSON instance for Status. In line 59, it should parse "certified" as Certified, not Succeeded.
- handle "certified" = pure Succeeded
+ handle "certified" = pure Certified
| instance ToSchema Status where | ||
| declareNamedSchema _ = do | ||
| let values = ["queued", "failed", "succeeded", "certified", "ready-for-certification","aborted"] :: [Value] | ||
| return $ NamedSchema (Just "RunStatus") $ mempty | ||
| & type_ ?~ SwaggerString | ||
| & enum_ ?~ values | ||
|
|
There was a problem hiding this comment.
The Swagger schema for Status is correctly defined, but it would be better to generate the list of values programmatically from the Status type itself. This way, if new statuses are added or existing ones are removed, the Swagger schema will automatically stay up-to-date.
let values = map (toJSON . show) [minBound .. maxBound :: Status] :: [Value]
| , ("reportContentId", textSchema) | ||
| ] | ||
| & required .~ [ "runId", "created", "utcSchema", "repoUrl" | ||
| , "commitDate","commitHash", "runStatus", "certificationPrice"] |
There was a problem hiding this comment.
In the required fields list for the Swagger schema of Run, "utcSchema" is included, which seems like a mistake. It should probably be "syncedAt". Please verify this.
- , "commitDate","commitHash", "runStatus", "certificationPrice"]
+ , "commitDate","commitHash", "runStatus", "certificationPrice", "syncedAt"]
| instance IsLabel "profileId" (ID Profile -> Profile -> Profile) where | ||
| fromLabel v p = p { profileId = v} |
There was a problem hiding this comment.
The IsLabel instance for "profileId" seems unnecessary and could potentially introduce confusion. If you need to update the profileId of a Profile, consider using record syntax or lenses directly instead of OverloadedLabels.
| @@ -685,6 +687,7 @@ | |||
| "async".components.library.planned = lib.mkOverride 900 true; | |||
| "http-media".components.library.planned = lib.mkOverride 900 true; | |||
| "word8".components.library.planned = lib.mkOverride 900 true; | |||
| "http-conduit".components.library.planned = lib.mkOverride 900 true; | |||
There was a problem hiding this comment.
The http-conduit library has been marked as planned. This means it will be built even if it's not required by any other package. Ensure that this is the intended behavior.
| @@ -380,6 +220,7 @@ dapps = table "dapp" | |||
| createTables :: MonadSelda m => m () | |||
| createTables = do | |||
| createTable certifications | |||
| createTable onChainCertifications | |||
There was a problem hiding this comment.
A new table onChainCertifications is being created. Ensure that this table's structure is defined correctly and that it meets your application's requirements.
| @@ -389,3 +230,4 @@ createTables = do | |||
| createTable tiers | |||
| createTable tierFeatures | |||
| createTable subscriptions | |||
| createTable l1Certifications | |||
There was a problem hiding this comment.
A new table l1Certifications is being created. Ensure that this table's structure is defined correctly and that it meets your application's requirements.
| let cert = Certification def txId time | ||
| certId <- insertWithPK certifications [cert] | ||
| -- and now add a l1Certification | ||
| let l1Cert = L1Certification runId certId | ||
| _ <- insert l1Certifications [l1Cert] | ||
| pure $ Just (L1CertificationDTO l1Cert (#certId certId cert)) |
There was a problem hiding this comment.
The logic of the createL1Certificate function has been modified. Now, after inserting a new certification, it also inserts a new L1Certification and returns an L1CertificationDTO. Ensure that this change aligns with the intended business logic and that the database schema supports these operations.
react-web/src/pages/certification/components/ResultContainer.test.tsx
Outdated
Show resolved
Hide resolved
react-web/src/pages/certification/Certification.helper.test.tsx
Outdated
Show resolved
Hide resolved
| , createAuditorReport = \dryRun reportInput (profileId,_) -> withEvent eb CreateAuditorReport \ev -> do | ||
| validateFeature L2UploadReport profileId | ||
| addField ev $ CreateAuditorReportFieldProfileId profileId | ||
| addField ev $ CreateAuditorReportDryRun (dryRun == Just True) | ||
| case dryRun of | ||
| Just True -> catch (createDraftMetadata reportInput) handleException | ||
| _ -> do | ||
| (fullMetadata,ipfs) <- catch (createMetadataAndPushToIpfs reportInput) handleException | ||
| addField ev $ CreateAuditorReportIpfsCid ipfs | ||
| pure fullMetadata | ||
| } | ||
| where | ||
| handleException :: (MonadError ServerError m ) => SomeException -> m a | ||
| handleException e = do | ||
| throwError err400 { errBody = LSB.pack $ show e } |
There was a problem hiding this comment.
A new function createAuditorReport has been added. This function seems to handle exceptions, but it only catches SomeException, which might not be ideal as it will catch all exceptions, including asynchronous exceptions like ThreadKilled or UserInterrupt, which are usually better left unhandled. Consider catching specific exceptions that you expect can occur during the execution of your code.
- handleException :: (MonadError ServerError m ) => SomeException -> m a
+ handleException :: (MonadError ServerError m ) => SpecificException -> m a
src/Plutus/Certification/Metadata.hs
Outdated
| hashURL :: MonadIO m => String -> m (Digest Blake2b_256) | ||
| hashURL url' = hashContent <$> simpleHttp (toHttpUrl url') | ||
|
|
||
| hashContent :: LBS.ByteString -> Digest Blake2b_256 | ||
| hashContent = hashWith Blake2b_256 . LBS.toStrict | ||
|
|
||
| throwError :: String -> c | ||
| throwError = throw . userError | ||
|
|
||
| -- TODO: post to ipfs if it's the | ||
| toReport :: MonadIO m => [ReportURL] -> m Report | ||
| toReport [] = throwError "No report URLs provided" | ||
| toReport urls = do | ||
| xs <- mapM (hashURL . show . unReportURL) urls | ||
| if allSame xs | ||
| then pure $ Report urls ( digestToHash $ head xs) | ||
| else throwError "Report URLs do not match" |
There was a problem hiding this comment.
The function hashURL is used to hash the content of a URL. However, it does not handle potential exceptions that might occur during the HTTP request (e.g., network errors, timeouts). Consider adding error handling to ensure the function behaves correctly in case of failures.
hashURL :: MonadIO m => String -> m (Digest Blake2b_256)
hashURL url' = do
result <- try $ simpleHttp (toHttpUrl url')
case result of
Left e -> throwError $ "Failed to fetch URL: " ++ show (e :: SomeException)
Right content -> return $ hashContent content
src/Plutus/Certification/Metadata.hs
Outdated
| toReport [] = throwError "No report URLs provided" | ||
| toReport urls = do | ||
| xs <- mapM (hashURL . show . unReportURL) urls | ||
| if allSame xs | ||
| then pure $ Report urls ( digestToHash $ head xs) | ||
| else throwError "Report URLs do not match" |
There was a problem hiding this comment.
The function toReport throws an error if the hashes of the URLs are not all the same. This could lead to unexpected crashes if the input contains different URLs. It would be better to return an error type instead of throwing an exception.
toReport :: MonadIO m => [ReportURL] -> m (Either String Report)
toReport [] = return $ Left "No report URLs provided"
toReport urls = do
xs <- mapM (hashURL . show . unReportURL) urls
if allSame xs
then return $ Right $ Report urls ( digestToHash $ head xs)
else return $ Left "Report URLs do not match"
src/Plutus/Certification/Metadata.hs
Outdated
| offchain <- createOffchainMetadata input | ||
| finalOffchain <- addIpfsToMetadataIfNecessary offchain | ||
| ipfsCid <- uploadToIpfs finalOffchain | ||
| let onchain = createOnchainMetadata input $ Just (finalOffchain,[toMetadataUrl ipfsCid]) | ||
| pure (FullMetadata (onchain,finalOffchain),IpfsCid ipfsCid) | ||
| where | ||
| addIpfsToMetadataIfNecessary (offchain :: OffChainMetadata) = do | ||
| let Report{reportURLs} = offchain.report | ||
| -- if there isn't any ipfs url in the report, add the ipfs url to the metadata | ||
| if Prelude.any (isIpfsUrl . unReportURL) reportURLs | ||
| then pure offchain | ||
| else do | ||
| -- get the ipfs cid of the first url | ||
| let url' = show . unReportURL . head $ offchain.report.reportURLs | ||
| bs <- simpleHttp (toHttpUrl url') | ||
| ipfsCid <- uploadToIpfsBS (LBS.toStrict bs) | ||
| let reportUrl = ReportURL $ parseURIUnsafe ("ipfs://" <> Text.unpack ipfsCid) | ||
| pure $ offchain { report = (offchain.report) { reportURLs = reportURLs <> [reportUrl] }} | ||
| isIpfsUrl uri = case uriScheme uri of | ||
| "ipfs:" -> True | ||
| _ -> False | ||
|
|
||
| toMetadataUrl :: Text -> MetadataUrl | ||
| toMetadataUrl ipfsCid = MetadataUrl $ parseURIUnsafe $ "ipfs://" <> Text.unpack ipfsCid | ||
|
|
||
| uploadToIpfs :: (MonadIO m, ToJSON a) => a -> m Text | ||
| uploadToIpfs obj = do | ||
| uploadToIpfsBS (LBS.toStrict $ encode obj) | ||
|
|
||
| uploadToIpfsBS :: (MonadIO m) => BS.ByteString -> m Text | ||
| uploadToIpfsBS bs = do | ||
| resp <- IPFS.uploadReportToIpfs IPFS.apiKey bs | ||
| case resp of | ||
| Left (IPFS.DecodeFailure _ err) -> throwError err | ||
| Left (IPFS.HttpError resp') -> | ||
| let (Status code msg) = responseStatus resp' | ||
| err = "IPFS gateway error code: " <> show code <> ", msg: " <> BS.unpack msg | ||
| in throwError err | ||
| Right (IPFS.UploadResponse (IpfsCid ipfsCid) _)-> pure ipfsCid |
There was a problem hiding this comment.
The function createMetadataAndPushToIpfs uploads data to IPFS but does not handle potential exceptions that might occur during the upload process (e.g., network errors, timeouts). Consider adding error handling to ensure the function behaves correctly in case of failures.
uploadToIpfsBS :: (MonadIO m) => BS.ByteString -> m (Either String Text)
uploadToIpfsBS bs = do
result <- try $ IPFS.uploadReportToIpfs IPFS.apiKey bs
case result of
Left e -> return $ Left $ "Failed to upload to IPFS: " ++ show (e :: SomeException)
Right resp ->
case resp of
Left (IPFS.DecodeFailure _ err) -> return $ Left err
Left (IPFS.HttpError resp') ->
let (Status code msg) = responseStatus resp'
err = "IPFS gateway error code: " <> show code <> ", msg: " <> BS.unpack msg
in return $ Left err
Right (IPFS.UploadResponse (IpfsCid ipfsCid) _) -> return $ Right ipfsCid
src/Plutus/Certification/Metadata.hs
Outdated
| parseURIUnsafe :: String -> URI | ||
| parseURIUnsafe = fromMaybe (throwError "Invalid URI") . parseURI | ||
|
|
There was a problem hiding this comment.
The function parseURIUnsafe throws an error when it fails to parse a URI. It would be better to return an error type instead of throwing an exception.
parseURIUnsafe :: String -> Either String URI
parseURIUnsafe str = case parseURI str of
Nothing -> Left "Invalid URI"
Just uri -> Right uri
There was a problem hiding this comment.
Files selected for processing (1)
- react-web/src/pages/certification/components/SuccessCard.tsx (2 hunks)
Files skipped from review due to trivial changes (1)
- react-web/src/pages/certification/components/SuccessCard.tsx
Review comments generated (0)
- Actionable : 0
- LGTM (Skipped): 0
- Noise Suppression (Skipped): 0
Tips
Chat with CodeRabbit Bot (@coderabbitai)
- Tag the bot with
@coderabbitaiin any review comment to ask it questions. The bot will not respond unless explicitly tagged. - A review comment is a comment on a diff or a file and not a comment on the PR itself.
- You can use the mouse to select multiple lines of code to provide exact context to the bot.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
@coderabbitai: Can you review the file that you skipped? |
|
Have couple test failures which will be taken care of during the revamp tickets. Merging this PR for now. |
https://input-output.atlassian.net/browse/PLT-5551
Summary by CodeRabbit
Release Notes
CreateAuditorReportfor creating auditor reports.Counterclass in Python code to be thread-safe, preventing data races.data-testidattributes to various React components and introducing new test cases.parseHTMLContentsfunction to match closing</h2>tag correctly.@coderabbitai: ignore