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

e2e asset problems #3653

Open
jsnoble opened this issue Jun 12, 2024 · 4 comments
Open

e2e asset problems #3653

jsnoble opened this issue Jun 12, 2024 · 4 comments

Comments

@jsnoble
Copy link
Member

jsnoble commented Jun 12, 2024

In the e2e fixtures are zip files of assets. These are old legacy assets to do not work with the latest changes to teraslice. In the code most of it was to test the asset structure and loading of operations which is not really where this test should live. It should live in job-components.

We decided (me, Austin, Peter, Joseph) to bypass those tests that reference those assets for now. We need to later review which tests should live in e2e vs job-components (assuming we don't have those types of tests in job-components) and re-enable tests that are still useful to in e2e.

@busma13
Copy link
Contributor

busma13 commented Jun 18, 2024

There were 10 tests disabled across 2 spec files.

  • There are 4 tests in simple-spec.ts.

    • 2 of these are testing different varieties of old style, non bundled assets. These don't seem useful anymore and I'd like to remove them.
    • 1 tests whether a newer asset will be used if there is more than one version of an asset.
    • The last test checks if you get the correct asset when specifying the version within the jobSpec. I'd like to tweak this to use the older asset, since the previous test proves that the newer asset will be used by default.
    • 2 new style bundled assets are needed to get these last 2 tests working(same asset with different version numbers).
  • There are 6 tests in api-spec.ts.

    • 4 test api endpoints related to assets, so we need to have 2 versions of the same asset uploaded.
    • 2 test job configs and just happen to use an example asset.
    • All 6 can be fixed by using the newer style assets needed for simple-spec.ts.

Feedback on these potential changes would be appreciated.

@godber
Copy link
Member

godber commented Jun 18, 2024

For the simple-spec.ts case:

  • Add the two new bundled assets and update the tests they would use.
  • For the old non-bundled assets, I think it's important that we make sure that those assets can no longer be uploaded to Teraslice. If we leave that functionality in there, it may get accidentally used. So my question on this, right now, if we upload a non-bundled or old style asset, do they work? Ideally they should be rejected at submission time.

@busma13
Copy link
Contributor

busma13 commented Jun 18, 2024

Non-bundled assets can be successfully uploaded to teraslice. But when you try to register a job that uses the asset you will get the same module_not_found error that makes the tests fail currently.

@godber
Copy link
Member

godber commented Jun 18, 2024

Side note:
I think @busma13 and I just realized that the reason existing assets work with the new esm build of teraslice (2.0.0) is because esbuild bundles both CJS and ESM style in the generated index.js.

godber pushed a commit that referenced this issue Jun 28, 2024
This PR makes the following changes:
- Replace old style assets in e2e tests with bundled assets.
- Re-enable tests that were failing because of those assets.
- Remove 2 tests that checked if different types of old style assets
could be used.

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

No branches or pull requests

3 participants