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

👍🏽 Create a folder of manifest files that broadly tests IF across a range of test cases positive and negative. #615

Closed
5 tasks done
Tracked by #629 ...
jmcook1186 opened this issue Apr 10, 2024 · 18 comments · Fixed by #660
Closed
5 tasks done
Tracked by #629 ...
Assignees
Milestone

Comments

@jmcook1186
Copy link
Contributor

jmcook1186 commented Apr 10, 2024

Sub of: #629 -> #650

prerequisites
#337

Why

As a user I want to have a substantial set of example manifests available to help me to understand how to use the various features of if and its plugins. As a core developer I also want to be able to use those example manifests as positive and negative test cases.

Context

We are implementing a more rigorous integration and regression test process. To do so, we need a substantial set of positive and negative test cases expressed as manifest files. These can overlap with the examples used to scaffold user onboarding. This ticket is for reviewing the existing set, fixing any that are broken, adding any that are missing, and tweaking to make sure they meet our needs for positive and negative testing. That will form the bulk of our automated unit, integration and regression testing.

It's good that we currently have manifests that demonstrate each feature of the IF and the standard library of plugins in isolation or close to isolation - these are quite commonly used to help debug people. It's also good that we have manifests with different pipelines so we can see that plugins integrate well with one another and work in context. So for the positive flows we should make sure:

  • every builtin has a dedicated manifest that demonstrates correct usage
  • every plugin has a dedicated manifest that demonstrates its correct usage
  • there are >5 "pipeline" manifests of varying complexity, including manifests with large, deeply nested trees.

For the negative flows, we should ensure we cover the common failure modes, including:

  • missing/incorrect initialize block
  • naming mismatches between initialize and pipeline
  • missing global/node-level config for each plugin
  • interaction errors such as mock observations being executed too late in a pipeline

Suggested manifests

├── bugs
│   ├── aggregation-error-wrong-metric.yml
│   ├── azure-importer-ignoring-defaults.yml
│   ├── azure-importer-incorrect-calculation.yml
│   ├── initialize-error-no-config.yml
│   ├── initialize-error-no-path.yml
│   ├── initialize-error-no-plugins.yml
│   ├── input-error-missing-duration.yml
│   ├── pipeline-error-naming-mismatch.yml
│   ├── pipeline-error-uninitialized-plugin.yml
│   └── pipeline-ordering-error.yml
├── examples
│   ├── basic.yml
│   ├── generics.yml
│   ├── mock-cpu-util-to-carbon.yml
│   ├── nesting.yml
│   ├── pipeline-teads-sci.yml
│   ├── pipeline-with-aggregate.yml
│   └── pipeline-with-mocks.yml
├── features
│   ├── aggregate-horizontal.yml
│   ├── aggregate-vertical.yml
│   └── aggregate.yml
├── integrations
│   ├── cloud-metadata-divide-boavizta.yml
│   ├── mock-obs-groupby.yml
│   └── mock-obs-time-sync.yml
├── outputs
└── plugins
    ├── cloud-metadata
    │   ├── failure-invalid-vendor.yaml
    │   └── success.yml
    ├── coefficient
    │   ├── failure-invalid-config-input-param.yml
    │   └── success.yml
    ├── divide
    │   ├── failure-invalid-config-denominator.yml
    │   └── success.yml
    ├── groupby
    │   ├── failure-invalid-config-group.yml
    │   └── success.yml
    ├── mock-observations
    │   ├── failure-invalid-config-cpu-range.yml
    │   └── success.yml
    ├── multiply
    │   ├── failure-input-parameter-is-missing.yml
    │   └── success.yml
    ├── regex
    │   ├── failure-missing-input-param.yml
    │   └── success.yml
    ├── sci
    │   ├── failure-invalid-config-value.yml
    │   └── success.yml
    ├── sci-m
    │   ├── failure-invalid-default-emission-value.yml
    │   └── success.yml
    ├── shell
    │   ├── failure-invalid-command.yml
    │   └── success.yml
    ├── sum
    │   ├── failure-missing-input-param.yml
    │   └── success.yml
    ├── tdp-finder
    │   ├── failure-missing-input-param.yml
    │   └── success.yml
    └── time-sync
        ├── failure-config-start-later-end.yml
        └── success.yml

Statement of Work
The goal of this ticket is to create a clear set of manifest files that contain both positive and negative tests and store them in a folder.

Prerequisites
Since it's looking at negative cases it's dependent on #592
This ticket is about creating a core directory of manifest files, all the files in this folder are run using the proposed if-verify tool (#638) which will let us know which are failing (don't match) and which are passing (match).
the above are dependent on #600

Acceptance Criteria

  • A folder structure in IF repo which contains these folders and manifest files.
    • examples: These are the manifest files we generate for teaching, training, in our docs or we just provide as examples for people to learn from and leverage. (Only should contain plugins we maintain)
    • plugins: These are almost unit test equivalent manifest files, test an individual built-in plugin - helps isolate issues which relate to one plugin running in isolation. These are also useful manifest files to have in plugin documentations.
      features: Through our issue refinement process (Revise issue refinement/readiness process #613) as we develop features for IF the manifest files we use to drive that feature should be added to this folder. It should be named in such a way that it's clearly linked to the GitHub issue that it relates to. This forms the bulk of our regression testing, as we develop new features, we should not break old features, a failing manifest in this folder will tell us a new feature, has broken and old feature.
    • bugs: Every reported bug should have at least one manifest file that triggers it, once we fix this bug we store in the bugs folder with a way to ID it back to the GH Issue ID. For regression testing all these manifest files should pass to ensure we haven’t re-introduced and old bug during development.
    • integrations: positive and negative test cases for common combinations of plugins in pipelines
  • add the negative test cases you want to include in each plugin folder (no need to go nuts, just one in each folder would be ok for now)
  • add at least 2 more integrations using commonly used plugins
  • add 3 manifests to the features folder (maybe test the aggregate feature there)
  • add manifests from recent bugs (inherited from @jawache 's comment above) so I can test out our triage process
@jawache
Copy link
Contributor

jawache commented Apr 16, 2024

@jmcook1186 / @MariamKhalatova as per the discussion here: https://www.notion.so/grnsft/IF-Epic-Build-trust-in-the-impact-framework-ecosystem-3800ec4e76314709ba4362523e121485?pvs=4#941e4f95b44740f59b69cc2c0647eef2

  • The goal of this ticket is to create a clear set of manifest files that contain both positive and negative tests and store them in a folder.
  • Since it's looking at negative cases it's dependent on 👍🏼 Update IF to dump logs and errors to yaml file #592
  • This ticket is about creating a core directory of manifest files, all the files in this folder are run using the proposed if-verify tool (Build if-check #638) which will let us know which are failing (don't match) and which are passing (match).
  • That will form the bulk of our automated unit, integration and regression testing.

Acceptance criteria

  • A folder structure in IF repo which contains these folders and manifest files.
  • manifests: Documented and well defined manifest files that always have to pass in dev and main.
    • examples: These are the manifest files we generate for teaching, training, in our docs or we just provide as examples for people to learn from and leverage. (Only should contain plugins we maintain)
    • plugins: These are almost unit test equivalent manifest files, test an individual built-in plugin - helps isolate issues which relate to one plugin running in isolation. These are also useful manifest files to have in plugin documentations.
    • features: Through our issue refinement process (Revise issue refinement/readiness process #613) as we develop features for IF the manifest files we use to drive that feature should be added to this folder. It should be named in such a way that it's clearly linked to the GitHub issue that it relates to. This forms the bulk of our regression testing, as we develop new features, we should not break old features, a failing manifest in this folder will tell us a new feature, has broken and old feature.
    • bugs: Every reported bug should have at least one manifest file that triggers it, once we fix this bug we store in the bugs folder with a way to ID it back to the GH Issue ID. For regression testing all these manifest files should pass to ensure we haven’t re-introduced and old bug during development.
  • if-verify tool runs on this folder and informs us if the manifests match or don't.

Once it's updated to the new format with acceptance criteria added i'll 👍🏽

@jawache jawache changed the title Review and refine the current set of example manifests Create a folder of manifest files that broadly tests IF across a range of test cases positive and negative. Apr 16, 2024
@jmcook1186
Copy link
Contributor Author

@jawache updated the issue to the new format and incorporated your comments

@jawache jawache changed the title Create a folder of manifest files that broadly tests IF across a range of test cases positive and negative. 👍🏽 Create a folder of manifest files that broadly tests IF across a range of test cases positive and negative. Apr 22, 2024
@zanete zanete moved this to Backlog in IF Apr 22, 2024
@zanete zanete moved this from Backlog to Ready in IF Apr 22, 2024
@zanete zanete moved this from Ready to Blocked in IF Apr 23, 2024
@zanete zanete added the blocked The issue is blocked and cannot proceed. label Apr 23, 2024
@jmcook1186 jmcook1186 moved this from Blocked to Ready in IF Apr 23, 2024
@jmcook1186
Copy link
Contributor Author

jmcook1186 commented Apr 23, 2024

Unblocked after chat with Asim - this is ready and I'll work on it from this afternoon.
I'll provide a PR containing new manifests covering examples, bugs and features. Then it can be reassigned to @narekhovhannisyan to build if-verify to act on those manifests.

@jmcook1186 jmcook1186 linked a pull request Apr 23, 2024 that will close this issue
9 tasks
@zanete zanete removed the blocked The issue is blocked and cannot proceed. label Apr 23, 2024
@zanete zanete mentioned this issue Apr 23, 2024
2 tasks
@MariamKhalatova
Copy link
Contributor

MariamKhalatova commented Apr 24, 2024

@jawache @jmcook1186 Overall the idea is not bad, but there are some questions and points to discuss
For example,

  1. Bugs folder - It's good to keep and run those manifests, related to any issue, but as they are part of regression suite, it'll be better to store them under a folder, dedicated to the specific manifest. Because now we will have the coverage of 1 plugin spread across 3 different suites. and it will be hard to maintain them and scale.
  2. Features folder - will be better to have it as temporary. F.ex. if we have a feature A related to plugin X , then we can temporarily create a folder "Feature A", collect the manifests for ongoing testing, then after having the final version of feature A, we can move those manifests to Plugins folder (into plugin X).
    With the structure you offered, we will have a pile of outdated manifests.. F.ex. if we have feature A, then feature B which changes the logic of plugin X. Then You will have a lot of manifests for feature A and feature B, while manifests of feature A are not necessary anymore.
    Instead, we may consider the following structure
  • Examples folder - the same as you already implemented
  • Plugins folder - with sub-folders (plugin A, plugin B...)
  • Integrations folder - for complex pipelines
  • Temp (or features) - after feature is ready , these manifests are moved to appropriate folder (into plugins or integrations).
    What do you think about this?

@jmcook1186
Copy link
Contributor Author

I don't love the idea of temp folders, only because it requires us to adopt an overcomplicated process.

This set of manifests is intended to support our automated tests - for checking none of our established behaviours break when a PR is merged. We are not aiming for hundreds of manifests in these folders, just enough to cover our main positive and negative flows. New features that are still in development can be run manually by the developer and QA during the build process.

So I still think the simpler approach works fine. A bit of occasional gardening of the manifest folders is not the end of the world in those cases you mentioned where something gets obseleted.

@narekhovhannisyan
Copy link
Member

@jmcook1186 @MariamKhalatova in that case we can keep something in the middle.

Examples folder - the same as you already implemented
Plugins folder - with sub-folders (plugin A, plugin B...)
Integrations folder - for complex pipelines

Plugins can include both positive and negative flows for each plugin. This way it's easy to manage and find exact manifest.

@zanete zanete moved this from Ready to In Refinement in IF Apr 24, 2024
@jmcook1186
Copy link
Contributor Author

ok, except that we will also want to test negative flows for feature and pipeline manifests, which aren't accounted for in your proposed file structure. I still prefer the simple option, but hear your point about navigating to specific manifests. Maybe the solution is just to impose a file naming convention that makes them searchable.

For example, in bugs:

<feature-affected>-<bug-description>.yml

e.g. aggregate-wrong-metric-name.yml

Then if you need to find the negative test cases relating to the aggregate feature you just take bugs/aggregate*.yml.

For multi-plugin manifests we can prepend with pipeline rather than the feature name.

If there's a strong reason why this doesn't work and I'm not seeing it, please tell me - otherwise I'm still favouring the simple, bugs, features, examples structure.

@zanete
Copy link

zanete commented Apr 25, 2024

@jmcook1186 @MariamKhalatova @narekhovhannisyan discussed on the standup and @jmcook1186 will summarise his conclusion.

  • there were quite strong arguments for not having a separate bugs folder as that would require one manifest to live in multiple locations making it harder to maintain
  • the proposed structure is simpler for QA but not so simple for general usage
  • @MariamKhalatova will create her structure and that will make it easier @jmcook1186 to see what she means
  • @MariamKhalatova will need to create some more manifests and expand on the set

@jmcook1186
Copy link
Contributor Author

jmcook1186 commented Apr 26, 2024

I hear the arguments for QA being simpler when positive and negative cases are contained in a single folder but here's what I don't like about that:

  • as a normal user (not a QA or dev) wanting to find an example manifest, I want to be able to look in features or examples and know that the manifest I grab should run. This should be as frictionless as possible. All I need to do to get a working manifest is avoid the clearly named bugs folder.
  • The file naming is going to be irritating if we have a folder for a plugin and then positive and negative test cases together - for example, maybe we have: aggregate/aggregate.yml as the positive case, and then I want three negative cases: aggregate/aggregate-error-wrong-metric.yml, aggregate/aggregate-error-missing-config.yml and aggregate-error-not-initialized. The user that is only interested in working manifests now has to enter the right folder and identify the "good" one from all the bad ones.

How about this as a compromise:
Inside manifests:

|-pipelines
    |- basic
           |- basic.yml     // the working manifest!
            |- negative-test-cases // all negative cases in here!
                        |- naming-mistmatch.yml
                        |- uninitialized-plugin.yml
    |- generics
            |- basic.yml
            |- negative-test-cases
                        |- naming-mistmatch.yml
                        |- uninitialized-plugin.yml

... the other pipeline examples
    
|-features
    |- sum
        |- sum.yml
        |- negative-test-cases
                      |- naming-mistmatch.yml
                      |- uninitialized-plugin.yml

    |- multiply
        |- multiply.yml
        |- negative-test-cases
                      |- naming-mistmatch.yml
                      |- uninitialized-plugin.yml


... the other features

@MariamKhalatova @narekhovhannisyan

@MariamKhalatova
Copy link
Contributor

MariamKhalatova commented Apr 26, 2024

@jmcook1186 What if you have 10 different success manifests for the same plugin?
like success with A values , success with B values... so in any case we will have to differentiate them by naming
Also, if the folder name is 'aggregate' , then the manifest names may not contain the word 'aggregate' if it feels annoying, as the folder name already indicates that the manifests inside are about 'aggregate'

@jmcook1186
Copy link
Contributor Author

ok, @MariamKhalatova please could you sketch out your proposed version here as soon as possible (make this your highest priority task please) - I want to get a decision on this today if possible.

@MariamKhalatova
Copy link
Contributor

jmcook1186 Yes, my version is ready, so I will provide the PR as soon as I'm at my desk.

@jawache
Copy link
Contributor

jawache commented Apr 26, 2024

@jmcook1186 and @MariamKhalatova it's important for us to very quickly get some manifest files together in a folder, we need some manifest files in a folder just to we can finish off refining #638 and #637.

Can we get some in a folder so we can start seeing actual files, how they differ, what features we need for if-diff etc...

To clarity also, if-unofficial-plugins is going away and we are talking about merging most of the if-plugins as builtins so I would not get caught up too much into the issue of replicating manifests in multiple places. Let's get them all in this one place and then figure out the rest later. Eventually most of these "plugins" will actually just be "builtins" but for now, let's get them all in one place.

  • features: When we develop a feature we will have a set of manifest files related to that feature that tests it out and developers can use to drive that feature. Those should go in a features folder.

  • bugs: When we receive a bug report it should come with a manifest file that triggers the bug, those should go in the bugs folder (@MariamKhalatova for this first phase just collect all the manifest files that triggered the bugs you found during the last few months with the hackathon)

  • plugins: To test/demo each builtin/plugin individually (eventually post all the migrations, this will only contain builtins, but for now just put all of them in there)

  • examples: This is the more complex pipelines (or simpler pipelines) manifests we believe form good learning examples that people can run to better understand the functionally of if.

  • To begin @jmcook1186 let's just organize our existing manifest files into the above structure with the naming convention you proposed.

  • @MariamKhalatova can you add to this folder the manifest files for the bugs you found during the last few months, I'd like to see what that looks like and also, how many bugs did we find? @jmcook1186 I'd also be interested in triaging them using our new categorization criteria, it will test for our new triage process and the ideas regarding how to get community contributors to fix them.

  • Then @MariamKhalatova and @jmcook1186 let's create a couple of negative case manifest files (3-5 or so is fine for now), just to see how that would look and how this might impact the requirements for if-diff.

After that I think we will have enough to use to try to think through what the requirements are for if-diff and if-check, the priority is getting enough manifest files together to refine those issues.

Then let's decide how to structure it, but talking about the folder structure now seems too early and a bit of a waste of time, the focus should be on actually getting some manifest files in a folder, we can always move them around later and change the folder structure as needs arise in the future.

@jmcook1186
Copy link
Contributor Author

Hi @jawache - I already organized the existing manifests into the proposed structure and created negative cases in PR #660

So we have that as a reference point, at least.

@MariamKhalatova
Copy link
Contributor

MariamKhalatova commented Apr 26, 2024

@jawache @jmcook1186 Okay. This conversation went a bit long, hope it was somehow useful. I just thought that my suggestions as QA specialist may be valuable for the final decision. Anyway, here is the structure suggested by me
#671
I already reviewed Joseph's PR , seems that Plugins folder is missing. Will leave a comment on the PR.
BTW, just to clarify. If a bug is related to a feature and at the same to a plugin, then in which folder should the manifest be added ? Only in bugs folder or also in plugins folder?
@jawache Regarding the bugs that we had, I haven't kept all the manifests. We can partially try to find some , mostly based on the bugs reported on the board.

@jmcook1186
Copy link
Contributor Author

jmcook1186 commented Apr 26, 2024

hi @MariamKhalatova thnks for the draft PR - I see what you mean. I like that examples is just fully working manifests. How would having negative test cases in in the integrations folder work though - there would be a lot of confusingly names files in there.

Also, I think the bugs folder is useful for the reason @jawache suggested above - when we receive a bug report we can copy the manifest in there.

So here's what I propose:

  • examples // here we have good onboarding manifests we can direct people to

    • demo-pipeline-1.yml
    • demo-pipeline-2.yml
    • demo-pipeline-3.yml
  • bugs // here we collect manifests from bug reports and issues we surface ourselves

    • bug-1.yml
    • bug-2.yml
  • features // here we have a folder per feature, +ve and -ve cases in folder

    • aggregate
      • success-1.yml
      • failure-1.yml
  • plugins // here we have a folder per plugin, +ve and -ve cases in folder

    • sci
      • success-1.yml
      • failure-1.yml
  • integrations // here we have +ve and -ve cases for integrations

    • pipeline-x-success.yml
    • pipeline-x-fail.yml
    • pipeline-y-success.yml
    • pipeline-y-fail.yml

To me this includes the best of both worlds as it gives you what you need for QA purposes but I won't worry that normal users will get lost and be unable to find good demo manifests easily. And it also supports our bug processing.
examples is for users, bugs is for us, and features, plugins and integrations are for automated testing.

I think this is what we should do. Let's start with this format and if some refinement is needed later then so be it, but this will get us off the ground and unblocked on if-diff.

@jmcook1186
Copy link
Contributor Author

jmcook1186 commented Apr 26, 2024

p.s. @MariamKhalatova @zanete

I've pushed changes to my PR #660 that implement the above folder structure and populated the folders with example manifests.

@MariamKhalatova you can check out that PR and add changes there. Specifically the 4 new tasks in the description of this issue

This doesn't have to be comprehensive now - it just has to contain enough for the development of if-diff and the automated testing flows to begin. We can follow up with a separate ticket to expand the coverage later.

@MariamKhalatova
Copy link
Contributor

@jmcook1186 Okay, will do.

@zanete zanete moved this from In Refinement to In Progress in IF Apr 29, 2024
@MariamKhalatova MariamKhalatova moved this from In Progress to Pending Review in IF May 1, 2024
@github-project-automation github-project-automation bot moved this from Pending Review to Done in IF May 1, 2024
@zanete zanete added this to the Improve Trust milestone Jun 5, 2024
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 a pull request may close this issue.

5 participants